MkDocs preview: https://95f6d2d7.dd-docs-preview.pages.dev

Fern preview: https://nvidia-preview-pr-625.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

Review: PR #625 — refactor: remove ModelProviderRegistry.default and require ModelConfig.provider

Summary

Final cleanup of the #589 deprecation cycle: drops the implicit "default provider" concept end-to-end. ModelConfig.provider goes from str | None = None to required str; ModelProviderRegistry.default (and both its validators + get_default_provider_name()) is removed; the CLI's set_default/get_default/"Change default provider" workflow is gone; agent introspection drops default_provider/effective_provider. Net −722 lines across 31 files, test suite updated in lockstep (deprecation-warning tests → strict-validation tests). Migration for on-disk YAMLs carrying a legacy default: relies on pydantic v2's extra="ignore" default, locked in by two new tests.

The PR is well-scoped, well-tested, and architecturally clean. The dependency direction (interface → engine → config) is preserved and the two ModelProviderRegistry classes (engine + CLI repo) are updated consistently. CI is green on the Python 3.10–3.12 matrix; 3.13 runs are still in progress at review time.

The primary risk is a hard public-API break, gated on the #594 deprecation cycle having shipped in a release — which the PR author has correctly flagged as a blocker and the release history confirms has not yet happened (last release v0.5.9, 2026-04-28; #594 merged 2026-05-05).

Findings

⛔ Blocker (already called out by author)

• Deprecation cycle not yet in a release. feat(models): deprecate implicit default provider routing #594 introduced the DeprecationWarning on 2026-05-05; the latest release tag is v0.5.9 (2026-04-28). Per issue refactor: remove ModelProviderRegistry.default and require ModelConfig.provider #590's own criterion, users must have had at least one release to see the warnings. This PR MUST NOT merge before the next release ships with the feat(models): deprecate implicit default provider routing #594 warnings intact. The author has pinned this at the top of the PR body — noting here only to reinforce that the gate is real and observable.

⚠️ Breaking changes worth an explicit user-facing note

• ModelConfig.provider becomes required. Anyone constructing ModelConfig(alias=..., model=...) without provider= now gets ValidationError. Likewise any on-disk ~/.data-designer/model_configs.yaml entry that omitted provider: or serialised it as null will fail to load after upgrade. The PR body notes "Migration is one-line per call site" for code, but on-disk configs from pre-feat(models): deprecate implicit default provider routing #594 users have no migration helper — they will hit ValidationError on load. Consider either (a) adding a CHANGELOG/release-notes entry calling this out, or (b) catching this specific ValidationError in ProviderRepository.load / ModelRepository.load with a targeted error message pointing at the fix. I lean toward (a) since users on stale model configs are a shrinking set and a clear CHANGELOG entry is cheaper than a bespoke migration path.

• Agent introspection shape change. get_model_aliases_state() drops default_provider from the top level and collapses configured_provider + effective_provider into a single provider field (see packages/data-designer/src/data_designer/cli/utils/agent_introspection.py). This is observable output that agents consuming data-designer agent context parse. The PR description lists this under "Removed" but doesn't flag it as a schema change. If any downstream agent scaffolding keys off those three field names, it will silently produce missing-field errors or empty strings. Worth an explicit callout in release notes alongside the ModelConfig change.

✅ Migration path for legacy YAML default: — correct and well-tested

• Both ModelProviderRegistry classes (engine model_provider.py and CLI provider_repository.py) inherit from BaseModel directly, not ConfigBase (which overrides extra="forbid"). The PR correctly locks this in:
  • test_registry_ignores_legacy_default_key (engine)
  • test_load_silently_ignores_legacy_default_key (CLI repo)
• Subtle-but-important invariant: if someone later rebases these classes onto ConfigBase, the migration silently breaks (old YAMLs would start raising ValidationError on the stale default: key). A short comment on the class itself — "inherits BaseModel directly so extra='ignore' lets us drop legacy default: keys" — would protect against this. The docstring on ProviderRepository.load already says this; the class itself doesn't.

🟡 Minor / nits

• provider_controller.py menu order change. The pre-refactor code appended "exit" after the conditional "change_default" so it always rendered last. The new code inlines "exit" in the initial dict literal, which keeps it last only because there is now nothing else to append. Behaviour is identical today; flagging only because a future "add menu option" patch could accidentally push exit out of its conventional last-position slot. Not worth changing.

• Unused import cleanup is thorough — warn_at_caller removed from models.py, default_model_settings.py, model_provider.py, provider_controller.py, provider_repository.py. Good. No orphaned warnings import.

• scripts/benchmarks/benchmark_engine_v2.py correctly updated to drop default_provider_name=. Nice catch — benchmark scripts are easy to miss.

• Docs parity: Four docs/concepts/models/*.md files had their deprecation warnings stripped. Consistent. Didn't spot any orphaned #589 references in the docs tree, but a quick grep -r "589" docs/ before merging wouldn't hurt.

• CLI README.md drops the "default: nvidia" line from the example YAML. Good — keeps user-visible examples aligned with the new schema.

Test coverage observations

• Previously-added warning-emission tests (test_model_config_provider_none_emits_deprecation_warning, test_explicit_default_emits_deprecation_warning, test_handle_change_default_emits_deprecation_warning, etc.) have been correctly replaced by their post-cycle counterparts (ValidationError-based). No test appears to have been silently dropped.
• test_init_no_user_providers_loads_from_yaml is the right shape for the remaining DataDesigner.__init__ YAML-fallback path, replacing the older three-test cluster (test_init_user_supplied_providers_ignore_unrelated_yaml_default, test_init_user_supplied_providers_preserve_first_wins_over_yaml_default, test_init_yaml_default_emits_single_deprecation_warning). The first two were bug: default model provider leaks from YAML into DataDesigner constructor #588 regressions — those bugs can't reoccur once the default field is gone, so removal is correct.
• No new e2e tests, but the PR description's "N/A (no runtime behavior change beyond stricter validation)" is accurate: this is a pure API-surface tightening.

Security / performance

• No security implications. No credentials handling changed.
• No performance implications. One fewer model_validator on ModelConfig and three fewer on ModelProviderRegistry — negligible but nominally a micro-win.

Verdict

Approve pending release gate. The code change is clean, the migration strategy for existing YAMLs is sound and tested, and the CI signal is strong. The two items worth addressing before merge are non-blocking:

1. (Recommended) Add a CHANGELOG / release-notes entry explicitly calling out (a) ModelConfig.provider becoming required, and (b) the agent-introspection schema change. Users upgrading from v0.5.9 → next release with stale on-disk configs or downstream agent tooling will otherwise hit an undocumented break.
2. (Nit) Drop a one-line comment on both ModelProviderRegistry classes pinning the BaseModel-not-ConfigBase invariant that makes the legacy-default: migration work.

Both are worth doing but neither should block merge once the release gate lifts.

Greptile Summary

This PR completes the removal of the deprecated ModelProviderRegistry.default registry-level default provider that was deprecated in v0.6.0. ModelConfig.provider is now a required str field, all implicit-default routing is removed, and a migration guard (LegacyModelConfigMigrationError) is added to the ModelRepository so existing on-disk configs missing provider fail fast with a clear message rather than silently returning None.

• Engine/config layer: ModelProviderRegistry.default, its validators, and get_default_provider_name() are fully removed; resolve_model_provider_registry drops default_provider_name; pydantic v2 extra="ignore" silently drops legacy default: keys on load.
• CLI layer: Provider service drops set_default/get_default; provider controller drops the "Change default provider" menu entry; the model-form builder now raises distinct errors for "no providers" vs "multiple providers but none selected"; the agent introspection schema exposes only a single provider field per model alias item.
• Test suite: 162 tests pass; deprecated-path tests are replaced with new regression tests covering the required-provider enforcement and the legacy-YAML compatibility invariant.

Important Files Changed

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before["Before (deprecated path)"]
        A1["ModelConfig(alias=..., model=...)"] -->|"provider=None"| B1["ModelProviderRegistry.get_provider(None)"]
        B1 -->|"fallback to registry.default or providers[0]"| C1["ModelProvider resolved"]
    end

    subgraph After["After (this PR)"]
        A2["ModelConfig(alias=..., model=..., provider='openai')"] -->|"provider required"| B2["ModelProviderRegistry.get_provider('openai')"]
        B2 -->|"exact name lookup"| C2["ModelProvider resolved"]
        B2 -->|"name not found"| E2["UnknownProviderError"]
    end

    subgraph CLI["CLI model_repository.load()"]
        F["load_config_file()"] --> G{{"provider field present & non-null?"}}
        G -->|"No"| H["LegacyModelConfigMigrationError (descriptive message)"]
        G -->|"Yes"| I["ModelConfigRegistry.model_validate()"]
        I -->|"OK"| J["Registry returned"]
        I -->|"Other error"| K["return None (silent)"]
    end

    subgraph Compat["Legacy YAML Compatibility"]
        L["model_providers.yaml with 'default: foo'"] -->|"pydantic extra=ignore"| M["ModelProviderRegistry loaded, default key dropped"]
    end

_{Reviews (6): Last reviewed commit: "fix: block model config writes when lega..." | Re-trigger Greptile}

docs/concepts/models/model-configs.md:18

provider | str | No | Reference to the name of the Provider to use ... If not specified, one set as the default provider ...

model-configs.md still says provider is not required and falls back to a default. Since this PR removes that path, this doc should say Required: Yes and drop the default-provider fallback text.

Looks like the release gate in the PR body is still active: latest GitHub release is v0.5.9, published before #594 merged, so users have not yet had a released version with the deprecation warning. Assuming that gate still applies, this should wait for the next release before merging.

Thanks for the careful review. I pushed an updated branch and believe the comments are now addressed:

• docs/concepts/models/model-configs.md now marks provider as required and removes the default-provider fallback text. I also made the same update in the Fern docs page.
• The release gate is now satisfied: v0.6.0 shipped on 2026-05-13 and includes feat(models): deprecate implicit default provider routing #594, so users have had a release containing the deprecation warnings before this removal lands. I updated the PR body accordingly.
• The breaking-change notes are now explicit in both the PR body and docs: stale model_configs.yaml entries that omit provider or set it to null need an explicit provider name.
• The agent-context schema change is called out more precisely: consumers should read each model alias item's provider; the top-level default_provider and per-item configured_provider / effective_provider fields are no longer emitted.
• I added class docstrings on both ModelProviderRegistry classes documenting the BaseModel / extra="ignore" invariant for legacy default: YAML keys.
• I addressed Greptile's CLI diagnostic note by splitting the ModelFormBuilder.build_config error messages for "no providers configured" versus "multiple providers configured but none selected", with a focused test.

The branch is rebased onto current main. GitHub Actions, docs preview, DCO, and Greptile are all green on the latest commit. Could you re-review when you have a chance?

hey, sorry for the slow re-review here, I was out on PTO and this slipped. One migration edge still looks open: with an older model_configs.yaml entry that omits provider, ModelRepository.load() returns None, so ModelService.add() treats the file like an empty registry and overwrites it. I reproduced that with a legacy alias plus one new add, and the saved file only kept the new alias. Could we make this a clear migration error and block writes until the stale aliases are fixed?

Thanks for catching that — pushed a fix in 5615847.

ModelRepository.load() now raises a new LegacyModelConfigMigrationError when it encounters a model_configs.yaml entry missing the required provider field, instead of returning None. That stops ModelService.add() (and update/delete/list) from treating the file as an empty registry and overwriting stale aliases. The error message names the offending alias(es) and points users at editing the file or re-running data-designer config models after the fix.

Call sites that load the registry surface it directly:

• ModelController.run() catches it and prints the error before bailing out, so the CLI never reaches the add/update/delete handlers with a stale-but-non-empty file.
• agent_introspection._load_registry() wraps it as AgentIntrospectionError(code=\"legacy_model_config\") so agent context consumers see a structured failure instead of a silent empty registry.

Coverage added in test_model_repository.py::test_load_legacy_missing_provider_raises and test_model_service.py::test_add_blocks_when_legacy_config_missing_provider (the latter reproduces your scenario: legacy alias on disk + one new `add` → assert it raises and the on-disk file still contains the legacy alias). PTAL.