From c2c36c7a09ef572789f4fce7d58ead6c4ac4a66c Mon Sep 17 00:00:00 2001 From: Nabin Mulepati Date: Thu, 30 Apr 2026 15:25:54 -0600 Subject: [PATCH 1/2] fix(interface): don't leak YAML default provider into user-supplied list When a caller passes ``model_providers`` to ``DataDesigner.__init__``, the YAML's ``default:`` key from ``~/.data-designer/model_providers.yaml`` was still being applied to the resulting ``ModelProviderRegistry``. This caused two related problems: 1. Hard failure: if the YAML default named a provider absent from the user-supplied list, construction raised ``ValidationError: Specified default 'X' not found in providers list``. 2. Silent override: if the YAML default matched a non-first user-supplied provider, the documented "first wins" behavior was silently overridden. Gate the YAML lookup on ``model_providers is None`` so that user-supplied lists own their own default. Also expose ``model_provider_registry`` and ``run_config`` as public read-only properties on ``DataDesigner``, paired with the existing ``secret_resolver`` property and ``set_run_config`` setter; tests now use these instead of the underscore-prefixed attributes. Closes #588. Made-with: Cursor --- .../data_designer/interface/data_designer.py | 45 ++++++- .../tests/interface/test_data_designer.py | 121 +++++++++++++----- 2 files changed, 127 insertions(+), 39 deletions(-) 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..307b6c4c8 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) or from the YAML's ``default:`` key (when + falling back to the on-disk providers 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..2a1e7be8d 100644 --- a/packages/data-designer/tests/interface/test_data_designer.py +++ b/packages/data-designer/tests/interface/test_data_designer.py @@ -420,17 +420,72 @@ 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_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 +498,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 +516,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 +535,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 +544,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 +913,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 +952,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 ( From 5a9fc27cc7760e5f6181d56d88625f2b49b849f9 Mon Sep 17 00:00:00 2001 From: Nabin Mulepati Date: Thu, 30 Apr 2026 15:47:48 -0600 Subject: [PATCH 2/2] fix(interface): tighten model_provider_registry docstring; pin YAML-fallback path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on PR #591: - Clarify ``model_provider_registry`` docstring so it reflects the full fallback chain: user-supplied first → YAML default (when set) → first provider in the YAML list. - Add ``test_init_no_user_providers_uses_yaml_default`` to lock the YAML-fallback contract that the #588 fix preserved but didn't pin. Made-with: Cursor --- .../data_designer/interface/data_designer.py | 6 +-- .../tests/interface/test_data_designer.py | 38 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) 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 307b6c4c8..09ab43b0a 100644 --- a/packages/data-designer/src/data_designer/interface/data_designer.py +++ b/packages/data-designer/src/data_designer/interface/data_designer.py @@ -439,9 +439,9 @@ def model_provider_registry(self) -> ModelProviderRegistry: 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) or from the YAML's ``default:`` key (when - falling back to the on-disk providers list). + 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 diff --git a/packages/data-designer/tests/interface/test_data_designer.py b/packages/data-designer/tests/interface/test_data_designer.py index 2a1e7be8d..f3bc44044 100644 --- a/packages/data-designer/tests/interface/test_data_designer.py +++ b/packages/data-designer/tests/interface/test_data_designer.py @@ -475,6 +475,44 @@ def test_init_user_supplied_providers_preserve_first_wins_over_yaml_default( 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)