diff --git a/packages/data-designer/src/data_designer/interface/data_designer.py b/packages/data-designer/src/data_designer/interface/data_designer.py index 6e64ea91c..09ab43b0a 100644 --- a/packages/data-designer/src/data_designer/interface/data_designer.py +++ b/packages/data-designer/src/data_designer/interface/data_designer.py @@ -37,7 +37,7 @@ from data_designer.engine.compiler import compile_data_designer_config from data_designer.engine.dataset_builders.dataset_builder import DatasetBuilder from data_designer.engine.mcp.io import list_tool_names -from data_designer.engine.model_provider import resolve_model_provider_registry +from data_designer.engine.model_provider import ModelProviderRegistry, resolve_model_provider_registry from data_designer.engine.resources.person_reader import ( PersonReader, create_person_reader, @@ -150,11 +150,20 @@ def __init__( self._run_config = RunConfig() self._managed_assets_path = Path(managed_assets_path or MANAGED_ASSETS_PATH) self._person_reader = person_reader - self._model_providers = self._resolve_model_providers(model_providers) + # Only consult the YAML's `default:` key when we are also falling back to + # the YAML's `providers:` list. A user-supplied `model_providers` list + # owns its own default (first wins), so the YAML default must not leak + # in and either (a) hard-fail validation when the YAML names a provider + # absent from the supplied list or (b) silently override the + # documented first-wins ordering. See issue #588. + if model_providers is None: + self._model_providers = self._resolve_model_providers(None) + default_provider_name = get_default_provider_name() + else: + self._model_providers = self._resolve_model_providers(model_providers) + default_provider_name = None self._mcp_providers = mcp_providers or [] - self._model_provider_registry = resolve_model_provider_registry( - self._model_providers, get_default_provider_name() - ) + self._model_provider_registry = resolve_model_provider_registry(self._model_providers, default_provider_name) self._seed_reader_registry = SeedReaderRegistry(readers=seed_readers or DEFAULT_SEED_READERS) @property @@ -423,6 +432,32 @@ def secret_resolver(self) -> SecretResolver: """ return self._secret_resolver + @property + def model_provider_registry(self) -> ModelProviderRegistry: + """Get the resolved model provider registry. + + Returns: + The ModelProviderRegistry containing the providers and default + resolved at construction time. The default is taken from the + first user-supplied provider when ``model_providers`` was passed + to the constructor; otherwise from the YAML's ``default:`` key + when set, falling back to the first provider in the YAML list. + """ + return self._model_provider_registry + + @property + def run_config(self) -> RunConfig: + """Get the runtime configuration applied to dataset generation. + + Returns: + The active RunConfig instance. Note that ``RunConfig`` normalizes + some fields on construction (e.g., ``shutdown_error_rate`` becomes + ``1.0`` when ``disable_early_shutdown=True``), so the returned + object may not exactly equal the one originally passed to + ``set_run_config``. + """ + return self._run_config + def set_run_config(self, run_config: RunConfig) -> None: """Set the runtime configuration for dataset generation. diff --git a/packages/data-designer/tests/interface/test_data_designer.py b/packages/data-designer/tests/interface/test_data_designer.py index b59b8d669..f3bc44044 100644 --- a/packages/data-designer/tests/interface/test_data_designer.py +++ b/packages/data-designer/tests/interface/test_data_designer.py @@ -420,17 +420,110 @@ def test_init_with_path_object(stub_artifact_path, stub_model_providers): assert designer is not None +def test_init_user_supplied_providers_ignore_unrelated_yaml_default( + stub_artifact_path: Path, + stub_model_providers: list[ModelProvider], + stub_managed_assets_path: Path, +) -> None: + """Regression for #588: a YAML ``default:`` that names a provider absent + from a user-supplied ``model_providers`` list must not leak into + construction. + + Pre-fix this raised ``ValidationError: Specified default 'unrelated' not + found in providers list``. + """ + with patch.object(dd_mod, "get_default_provider_name", return_value="unrelated"): + data_designer = DataDesigner( + artifact_path=stub_artifact_path, + model_providers=stub_model_providers, + secret_resolver=PlaintextResolver(), + managed_assets_path=stub_managed_assets_path, + ) + + assert data_designer.model_provider_registry.get_default_provider_name() == "stub-model-provider" + + +def test_init_user_supplied_providers_preserve_first_wins_over_yaml_default( + stub_artifact_path: Path, + stub_managed_assets_path: Path, +) -> None: + """Regression for #588: when the YAML ``default:`` matches a user-supplied + provider that isn't first in the list, the documented ``model_providers[0]`` + "first wins" behavior must not be silently overridden. + """ + user_providers = [ + ModelProvider( + name="first-provider", + endpoint="https://first.example.com/v1", + api_key="FIRST_API_KEY", + ), + ModelProvider( + name="second-provider", + endpoint="https://second.example.com/v1", + api_key="SECOND_API_KEY", + ), + ] + + with patch.object(dd_mod, "get_default_provider_name", return_value="second-provider"): + data_designer = DataDesigner( + artifact_path=stub_artifact_path, + model_providers=user_providers, + secret_resolver=PlaintextResolver(), + managed_assets_path=stub_managed_assets_path, + ) + + assert data_designer.model_provider_registry.get_default_provider_name() == "first-provider" + + +def test_init_no_user_providers_uses_yaml_default( + stub_artifact_path: Path, + stub_managed_assets_path: Path, +) -> None: + """Pin the unchanged YAML-fallback path: when the caller omits + ``model_providers``, DataDesigner consults both ``providers:`` and + ``default:`` from the YAML. + + The fix in #588 only changes the user-supplied branch; this test locks the + YAML-fallback branch's contract so a future refactor can't silently regress + it. + """ + yaml_providers = [ + ModelProvider( + name="yaml-first", + endpoint="https://yaml-first.example.com/v1", + api_key="yaml-first-key", + ), + ModelProvider( + name="yaml-second", + endpoint="https://yaml-second.example.com/v1", + api_key="yaml-second-key", + ), + ] + + with ( + patch.object(dd_mod, "get_default_providers", return_value=yaml_providers), + patch.object(dd_mod, "get_default_provider_name", return_value="yaml-second"), + ): + data_designer = DataDesigner( + artifact_path=stub_artifact_path, + secret_resolver=PlaintextResolver(), + managed_assets_path=stub_managed_assets_path, + ) + + assert data_designer.model_provider_registry.get_default_provider_name() == "yaml-second" + + def test_run_config_setting_persists(stub_artifact_path, stub_model_providers): """Test that run config setting persists across multiple calls.""" data_designer = DataDesigner(artifact_path=stub_artifact_path, model_providers=stub_model_providers) # Test default values - assert data_designer._run_config.disable_early_shutdown is False - assert data_designer._run_config.shutdown_error_rate == 0.5 - assert data_designer._run_config.shutdown_error_window == 10 - assert data_designer._run_config.buffer_size == 1000 - assert data_designer._run_config.max_conversation_restarts == 5 - assert data_designer._run_config.max_conversation_correction_steps == 0 + assert data_designer.run_config.disable_early_shutdown is False + assert data_designer.run_config.shutdown_error_rate == 0.5 + assert data_designer.run_config.shutdown_error_window == 10 + assert data_designer.run_config.buffer_size == 1000 + assert data_designer.run_config.max_conversation_restarts == 5 + assert data_designer.run_config.max_conversation_correction_steps == 0 # Test setting custom values data_designer.set_run_config( @@ -443,12 +536,12 @@ def test_run_config_setting_persists(stub_artifact_path, stub_model_providers): max_conversation_correction_steps=2, ) ) - assert data_designer._run_config.disable_early_shutdown is True - assert data_designer._run_config.shutdown_error_rate == 1.0 # normalized when disabled - assert data_designer._run_config.shutdown_error_window == 25 - assert data_designer._run_config.buffer_size == 500 - assert data_designer._run_config.max_conversation_restarts == 7 - assert data_designer._run_config.max_conversation_correction_steps == 2 + assert data_designer.run_config.disable_early_shutdown is True + assert data_designer.run_config.shutdown_error_rate == 1.0 # normalized when disabled + assert data_designer.run_config.shutdown_error_window == 25 + assert data_designer.run_config.buffer_size == 500 + assert data_designer.run_config.max_conversation_restarts == 7 + assert data_designer.run_config.max_conversation_correction_steps == 2 # Test updating values data_designer.set_run_config( @@ -461,12 +554,12 @@ def test_run_config_setting_persists(stub_artifact_path, stub_model_providers): max_conversation_correction_steps=1, ) ) - assert data_designer._run_config.disable_early_shutdown is False - assert data_designer._run_config.shutdown_error_rate == 0.3 - assert data_designer._run_config.shutdown_error_window == 5 - assert data_designer._run_config.buffer_size == 750 - assert data_designer._run_config.max_conversation_restarts == 9 - assert data_designer._run_config.max_conversation_correction_steps == 1 + assert data_designer.run_config.disable_early_shutdown is False + assert data_designer.run_config.shutdown_error_rate == 0.3 + assert data_designer.run_config.shutdown_error_window == 5 + assert data_designer.run_config.buffer_size == 750 + assert data_designer.run_config.max_conversation_restarts == 9 + assert data_designer.run_config.max_conversation_correction_steps == 1 def test_run_config_normalizes_error_rate_when_disabled(stub_artifact_path, stub_model_providers): @@ -480,7 +573,7 @@ def test_run_config_normalizes_error_rate_when_disabled(stub_artifact_path, stub shutdown_error_rate=0.7, ) ) - assert data_designer._run_config.shutdown_error_rate == 0.7 + assert data_designer.run_config.shutdown_error_rate == 0.7 # When disabled, shutdown_error_rate should be normalized to 1.0 data_designer.set_run_config( @@ -489,7 +582,7 @@ def test_run_config_normalizes_error_rate_when_disabled(stub_artifact_path, stub shutdown_error_rate=0.7, ) ) - assert data_designer._run_config.shutdown_error_rate == 1.0 + assert data_designer.run_config.shutdown_error_rate == 1.0 def test_run_config_rejects_invalid_buffer_size() -> None: @@ -858,13 +951,12 @@ def test_create_logs_secure_jinja_rendering_mode( stub_sampler_only_config_builder: DataDesignerConfigBuilder, stub_managed_assets_path: Path, ) -> None: - with patch.object(dd_mod, "get_default_provider_name", return_value="stub-model-provider"): - data_designer = DataDesigner( - artifact_path=stub_artifact_path, - model_providers=stub_model_providers, - secret_resolver=PlaintextResolver(), - managed_assets_path=stub_managed_assets_path, - ) + data_designer = DataDesigner( + artifact_path=stub_artifact_path, + model_providers=stub_model_providers, + secret_resolver=PlaintextResolver(), + managed_assets_path=stub_managed_assets_path, + ) data_designer.set_run_config(RunConfig(jinja_rendering_engine=JinjaRenderingEngine.SECURE)) with ( @@ -898,13 +990,12 @@ def test_preview_logs_native_jinja_rendering_mode( stub_sampler_only_config_builder: DataDesignerConfigBuilder, stub_managed_assets_path: Path, ) -> None: - with patch.object(dd_mod, "get_default_provider_name", return_value="stub-model-provider"): - data_designer = DataDesigner( - artifact_path=stub_artifact_path, - model_providers=stub_model_providers, - secret_resolver=PlaintextResolver(), - managed_assets_path=stub_managed_assets_path, - ) + data_designer = DataDesigner( + artifact_path=stub_artifact_path, + model_providers=stub_model_providers, + secret_resolver=PlaintextResolver(), + managed_assets_path=stub_managed_assets_path, + ) data_designer.set_run_config(RunConfig(jinja_rendering_engine=JinjaRenderingEngine.NATIVE)) with (