Greptile Summary

This PR fixes a bug where DataDesigner.__init__ always consulted get_default_provider_name() from the YAML config — even when the caller supplied their own model_providers list — causing either a hard ValidationError or a silent override of the documented "first wins" ordering. The fix gates the YAML default lookup on model_providers is None, adds public model_provider_registry and run_config properties, removes a now-unnecessary test workaround, and adds three focused regression tests.

Important Files Changed

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[DataDesigner.__init__ called] --> B{model_providers is None?}
    B -- Yes --> C[resolve from YAML providers list]
    C --> D[get_default_provider_name from YAML]
    D --> E[resolve_model_provider_registry with YAML default]
    B -- No --> F[use caller-supplied providers list]
    F --> G[default_provider_name = None, first wins]
    G --> H[resolve_model_provider_registry, default=None]
    E --> I[ModelProviderRegistry ready]
    H --> I

_{Reviews (2): Last reviewed commit: "fix(interface): tighten model_provider_r..." | Re-trigger Greptile}

Code Review: PR #591 — fix(interface): don't leak YAML default provider into user-supplied list

Summary

The PR fixes issue #588: DataDesigner.__init__ was always consulting the default: key from ~/.data-designer/model_providers.yaml and passing it to resolve_model_provider_registry, even when the caller supplied their own model_providers list. That produced two failure modes:

1. Hard fail — when the YAML default: named a provider not in the user's list, pydantic validation raised ValidationError.
2. Silent override — when the YAML default: matched a non-first user-supplied provider, the documented "first wins" ordering was silently flipped.

The fix gates get_default_provider_name() on model_providers is None so the YAML default only applies when also falling back to the YAML providers: list. resolve_model_provider_registry already handles default_provider_name=None by falling back to model_providers[0].name (packages/data-designer-engine/src/data_designer/engine/model_provider.py:77), so the first-wins contract is preserved for user-supplied lists.

The PR also promotes _model_provider_registry and _run_config to public read-only @propertys, and updates existing tests to consume the public properties rather than the underscore-prefixed attributes.

Findings

Correctness — ✅ fix is sound

• The branching in __init__ correctly isolates the two cases: YAML fallback gets the YAML default; user-supplied list gets None, which resolve_model_provider_registry converts into model_providers[0].name. Matches the PR description and the engine-side helper.
• The two new regression tests (test_init_user_supplied_providers_ignore_unrelated_yaml_default, test_init_user_supplied_providers_preserve_first_wins_over_yaml_default) pin both repros from bug: default model provider leaks from YAML into DataDesigner constructor #588 directly and assert against model_provider_registry.get_default_provider_name() — a faithful reproduction of the bug conditions.
• Removing the patch.object(dd_mod, "get_default_provider_name", ...) workaround from the two jinja-rendering tests is correct: those patches only existed because an unrelated YAML default on the test host could cause construction to fail, which is exactly what this PR fixes. Removing them is appropriate cleanup, not test-scope creep.

Test coverage — ✅ adequate

• Both bug modes are covered (unrelated-name → hard fail; matching-non-first-name → silent override). The first-wins assertion in the second test is the important part.
• The test_run_config_* churn is a pure rename (._run_config → .run_config) and flows naturally from exposing the new property.
• Not covered but arguably could be: a negative test that confirms the YAML default: does still apply when model_providers is None (the happy fallback path). Not a blocker — that path is exercised implicitly elsewhere, and the PR is scoped narrowly.

Public API surface — minor consideration

Exposing model_provider_registry: ModelProviderRegistry makes the engine-layer type ModelProviderRegistry part of the interface package's public contract. The interface already re-exports engine types in other places (e.g., the model facade), so this is consistent with existing patterns, but worth flagging as an intentional widening of the public surface. The docstring is accurate about how default is resolved.

The run_config property is symmetric with the existing set_run_config setter, so exposing it as a read-only property is the right shape. The docstring correctly notes that RunConfig normalizes some fields on construction (e.g., shutdown_error_rate → 1.0 when disable_early_shutdown=True), which matches the behavior exercised in test_run_config_normalizes_error_rate_when_disabled.

Style / project conventions — ✅ clean

• Absolute imports, from __future__ import annotations in effect, modern type syntax (list[ModelProvider] | None) — all consistent with AGENTS.md / STYLEGUIDE.md.
• The comment inside __init__ explaining why the branch exists, with a link to bug: default model provider leaks from YAML into DataDesigner constructor #588, is exactly the kind of non-obvious-why comment the project encourages.
• No relative imports introduced; no heavy third-party import added; the new ModelProviderRegistry import is already part of the engine dependency flow (interface → engine).

Minor nits (non-blocking)

• _resolve_model_providers signature is now slightly awkward. It accepts list[ModelProvider] | None, but the caller in __init__ now branches on None before the call and passes either None or the actual list. The inner method still has the if model_providers is None: branch and a stray return model_providers or [] at the end that also handles None. The logic still works, but the None-handling is now split across caller and callee. A tiny follow-up could push the branch entirely into __init__ (since the caller already knows which case it's in) and tighten _resolve_model_providers to list[ModelProvider] | None → list[ModelProvider] with a cleaner single path. Not in scope for this PR.
• The old resolve_model_provider_registry(..., get_default_provider_name()) one-liner was re-formatted into a two-step assignment. The new form is clearer given the branching; no concern.
• Commit/PR description is thorough and calls out that new tests fail on main — good discipline.

Risks

• Low. The change is narrowly scoped, the bug is well-characterized, and the regression tests directly pin both repros. No behavior change for the YAML-fallback path (the only path that still reads get_default_provider_name()).
• No security implications — no new network, filesystem, or deserialization surface. The YAML file is still read only under the existing fallback path.
• No performance implications.

Verdict

Approve with no required changes. The fix is small, correct, well-tested, and addresses both failure modes described in #588. The API additions (model_provider_registry, run_config properties) are reasonable and symmetric with existing members. The removal of the two patch.object workarounds is appropriate now that the underlying bug is gone.

The only optional follow-up worth considering is tightening _resolve_model_providers so its None-handling isn't split across the caller and callee — but that's cosmetic and belongs in a separate change.

Thanks for the thorough write-up on this one, @nabinchha — the issue analysis in #588 made the fix easy to verify against intent.

Summary

The PR gates the YAML default: lookup on model_providers is None, so a user-supplied list owns its own first-wins default and never trips ModelProviderRegistry's default ∈ providers validator with an unrelated name from ~/.data-designer/model_providers.yaml. It also exposes model_provider_registry and run_config as read-only properties so tests (and external callers) don't have to reach into private attributes. The implementation matches the suggested fix in #588 exactly.

Findings

Suggestions — Take it or leave it

packages/data-designer/src/data_designer/interface/data_designer.py:436-446 — model_provider_registry docstring is slightly imprecise

• What: The "Returns" section says the default comes "from the YAML's default: key (when falling back to the on-disk providers list)". When the YAML has no default: key, the registry actually falls back to providers[0].name of the YAML list (via resolve_model_provider_registry), not the YAML default.
• Why: Minor — the fallback chain is "user-supplied first → user-supplied[0] → YAML default → YAML[0]", and the current wording elides the last step.
• Suggestion: Tighten to e.g. "from the YAML's default: key (when set), otherwise the first provider in the list."

packages/data-designer/src/data_designer/interface/data_designer.py:159-166 — branches could collapse

• What: Both branches call self._resolve_model_providers(model_providers) (passing None works fine in the user-supplied branch since _resolve_model_providers already handles None).
• Why: Mild duplication; not a real cost — KISS arguably favors the explicit form you have.
• Suggestion (optional):

  default_provider_name = get_default_provider_name() if model_providers is None else None
  self._model_providers = self._resolve_model_providers(model_providers)

  Take it or leave it — the current form reads clearly with the inline comment.

packages/data-designer/tests/interface/test_data_designer.py — no test pins the YAML-default-applies path

• What: The two new tests both cover the user-supplied branch. There's no regression test that asserts the intended behavior in the fallback branch — i.e., when model_providers is None and the YAML has default: X, the registry's default is X.
• Why: The fix preserves that path correctly today, but a future refactor could regress it silently. A small parallel test (mock the loader to return a 2-provider list with default pointing at the second) would lock that contract in.
• Suggestion: Optional — add a third test like test_init_no_user_providers_uses_yaml_default. Skip if you'd rather keep the PR minimal.

packages/data-designer/src/data_designer/interface/data_designer.py:449-459 — asymmetric run_config API

• What: We now have a run_config getter property and a set_run_config(...) method. The Pythonic pairing would be @run_config.setter, but that would change the public API.
• Why: Not for this PR, but worth flagging so a future cleanup can land alongside the #589/#590 work the issue mentions.
• Suggestion: Consider tracking the cleanup as a follow-up issue rather than touching it here.

What Looks Good

• The diagnosis trail is exemplary. The issue narrows the bug to specific source lines in three packages, lays out two concrete repros, and even points out the existing test-side patches that exist because of the bug — the PR then deletes those workarounds, which is a satisfying signal that the fix matches the cause.
• Minimal, surgical change. Two small edits to __init__, one inline comment that explains the why, two regression tests, two workaround removals. No drive-by refactors.
• Both failure modes are pinned. test_init_user_supplied_providers_ignore_unrelated_yaml_default covers the hard-fail path and test_init_user_supplied_providers_preserve_first_wins_over_yaml_default covers the silent-override path. The patches on get_default_provider_name are necessary (without them, the tests would be flaky regression guards on a clean machine where YAML has no default).
• Bonus robustness: by gating on model_providers is None, callers who supply their own providers no longer depend on the YAML file existing or being well-formed — the _get_default_providers_file_content FileNotFoundError path can no longer bite them.

Verdict

Ship it (with nits). Fix is correct, minimal, and well-tested. The suggestions above are all optional polish.

This review was generated by an AI assistant.

Thanks for the review @johnnygreco — pushed 5a9fc27 addressing the two suggestions I took:

• Docstring tightening (suggestion 1) — `model_provider_registry` now spells out the full fallback chain: user-supplied first → YAML `default:` (when set) → first provider in the YAML list.
• YAML-fallback pin test (suggestion 3) — added `test_init_no_user_providers_uses_yaml_default` so the unchanged-by-this-PR fallback contract is explicitly locked in. Future refactors of `init` can't silently regress it.

Skipped:

• Branch collapse (suggestion 2) — agreed with your KISS aside; the explicit form pairs more cleanly with the inline `# See issue bug: default model provider leaks from YAML into DataDesigner constructor #588.` comment.
• `run_config` setter symmetry (suggestion 4) — out of scope per your note; happy to file a follow-up if useful.