refactor: remove ModelProviderRegistry.default and require ModelConfig.provider#625
refactor: remove ModelProviderRegistry.default and require ModelConfig.provider#625nabinchha wants to merge 2 commits into
Conversation
|
MkDocs preview: https://c5716626.dd-docs-preview.pages.dev Fern preview: https://nvidia-preview-pr-625.docs.buildwithfern.com/nemo/datadesigner
|
Review: PR #625 —
|
Greptile SummaryThis PR completes the deprecation cycle started in #589/#594 by permanently removing
|
| Filename | Overview |
|---|---|
| packages/data-designer-engine/src/data_designer/engine/model_provider.py | Removes default field, three model-validators, and get_default_provider_name(). Adds validate_providers_not_empty field-validator. get_provider now requires a concrete str. resolve_model_provider_registry drops default_provider_name param. Clean and correct. |
| packages/data-designer-config/src/data_designer/config/models.py | Makes ModelConfig.provider required (str, no default). Removes the _warn_on_implicit_provider model-validator. Breaking API change, intentional and documented. |
| packages/data-designer/src/data_designer/interface/data_designer.py | Removes YAML default: consultation, get_default_provider_name() call, and the warnings.catch_warnings suppression block. Construction simplified to a single resolve_model_provider_registry call. |
| packages/data-designer/src/data_designer/cli/forms/model_builder.py | Splits the previously conflated 'no providers' error into two precise branches: one for zero providers and one for multiple-providers-but-none-selected. Fixes the misleading error message flagged in prior review. |
| packages/data-designer/src/data_designer/cli/utils/agent_introspection.py | Drops default_provider from state dict and collapses configured_provider/effective_provider to a single provider field per item. Direct mc.provider lookup replaces the effective = mc.provider or default_provider fallback. |
| packages/data-designer/src/data_designer/cli/utils/agent_text_formatter.py | Removes default_provider: header line from model-aliases text output and the column_labels parameter from _format_table. Now renders the provider column directly without remapping. |
| packages/data-designer/src/data_designer/cli/services/provider_service.py | Removes set_default/get_default methods and the registry.default tracking in update and delete. Straightforward deletions with no correctness concerns. |
| packages/data-designer/src/data_designer/cli/repositories/provider_repository.py | Removes default field from CLI-layer ModelProviderRegistry. Deprecation warning for legacy default: YAML key removed; pydantic extra="ignore" silently drops it. Adds doc comment explaining the invariant. |
| packages/data-designer/src/data_designer/cli/controllers/provider_controller.py | Removes _handle_change_default and the conditional 'Change default provider' menu insertion. 'exit' option is now unconditionally present in the options dict. |
| packages/data-designer/tests/interface/test_data_designer.py | Removes ~115 lines of deprecation-warning and first-wins behavioral tests that no longer apply. Retains a single test pinning the YAML-fallback path, which now just checks provider names rather than get_default_provider_name(). |
| packages/data-designer-engine/tests/engine/test_model_provider.py | Removes ~80 lines of deprecation-warning tests. Adds test_registry_ignores_legacy_default_key to pin pydantic's extra="ignore" behavior for old YAML payloads. All remaining assertions are correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DataDesigner.__init__] --> B{model_providers\nsupplied?}
B -- yes --> C[_resolve_model_providers\nuser list]
B -- no --> D[_resolve_model_providers\nload from YAML]
C --> E[resolve_model_provider_registry\nproviders only]
D --> E
E --> F[ModelProviderRegistry\nvalidate_providers_not_empty\nvalidate_unique_names]
F --> G[_model_provider_registry]
H[ModelConfig.provider\nrequired str] --> I[registry.get_provider\nname: str]
G --> I
I -- found --> J[ModelProvider]
I -- not found --> K[UnknownProviderError]
Reviews (4): Last reviewed commit: "Merge branch 'main' into nmulepati/fix-5..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| else: | ||
| provider = None | ||
| raise ValueError( | ||
| "Cannot build ModelConfig: no provider specified in form data and no " | ||
| "available providers configured. Add a provider first via " | ||
| "`data-designer config providers`." | ||
| ) |
There was a problem hiding this comment.
form_data. When len(self.available_providers) > 1 and no "provider" key is present, the user sees a misleading diagnostic telling them to add a provider when providers already exist.
| else: | |
| provider = None | |
| raise ValueError( | |
| "Cannot build ModelConfig: no provider specified in form data and no " | |
| "available providers configured. Add a provider first via " | |
| "`data-designer config providers`." | |
| ) | |
| else: | |
| if self.available_providers: | |
| raise ValueError( | |
| "Cannot build ModelConfig: multiple providers are configured but none " | |
| "was specified in form data. Specify provider= explicitly on each ModelConfig." | |
| ) | |
| raise ValueError( | |
| "Cannot build ModelConfig: no provider specified in form data and no " | |
| "available providers configured. Add a provider first via " | |
| "`data-designer config providers`." | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer/src/data_designer/cli/forms/model_builder.py
Line: 294-299
Comment:
The error message incorrectly states "no available providers configured" in a branch that also fires when multiple providers ARE configured but none was included in `form_data`. When `len(self.available_providers) > 1` and no `"provider"` key is present, the user sees a misleading diagnostic telling them to add a provider when providers already exist.
```suggestion
else:
if self.available_providers:
raise ValueError(
"Cannot build ModelConfig: multiple providers are configured but none "
"was specified in form data. Specify provider= explicitly on each ModelConfig."
)
raise ValueError(
"Cannot build ModelConfig: no provider specified in form data and no "
"available providers configured. Add a provider first via "
"`data-designer config providers`."
)
```
How can I resolve this? If you propose a fix, please make it concise.
| model: str | ||
| inference_parameters: InferenceParamsT = Field(default_factory=ChatCompletionInferenceParams) | ||
| provider: str | None = None | ||
| provider: str |
There was a problem hiding this comment.
Claude and Codex both caught a migration edge here: once provider is required, an existing model_configs.yaml with one provider-less entry makes ModelRepository.load() return None, so CLI and agent flows can treat the whole model file as missing. Could we add a migration/error path in ModelRepository.load() so users see "add provider to alias X" instead of silently losing the registry?
|
|
Looks like the release gate in the PR body is still active: latest GitHub release is |
andreatgretel
left a comment
andreatgretel
left a comment
There was a problem hiding this comment.
Requesting changes based on the migration issue and stale docs noted above. The release gate in the PR body also still looks active.
nabinchha
force-pushed
the
nmulepati/fix-590-remove-implicit-default-provider
branch
2 times, most recently
from
f97431c to
eab4026
Compare
…g.provider (#590) Final cleanup after the #589 deprecation cycle. Providers are now a named list referenced by name end-to-end — no hidden registry default, no YAML default leaking into DataDesigner construction (#588), and no ambiguity about which provider wins. ModelConfig.provider becomes a required str. ModelProviderRegistry.default and the associated validators, get_default_provider_name() helper, CLI "Change default provider" workflow, and the "Default" column in `data-designer config list` are removed. resolve_model_provider_registry() drops its default_provider_name kwarg, and DataDesigner.__init__ no longer threads a YAML default through registry construction. Pydantic v2's extra="ignore" default lets existing on-disk YAMLs with default: keys load cleanly, so users on the deprecation cycle migrate without breakage.
nabinchha
force-pushed
the
nmulepati/fix-590-remove-implicit-default-provider
branch
from
eab4026 to
09eeae7
Compare
|
Thanks for the careful review. I pushed an updated branch and believe the comments are now addressed:
The branch is rebased onto current |
2 participants
Summary
Final cleanup after the #589 deprecation cycle. Providers are now a named list referenced explicitly by each
ModelConfig: no hidden registry default, no YAMLdefault:flowing intoDataDesigner.__init__, and no ambiguity about which provider wins.The original release gate is now satisfied:
v0.6.0shipped on 2026-05-13 and includes #594, so users have had a release containing the deprecation warnings before this removal lands.Related Issue
Closes #590. Depends on #588 (merged as #591) and #589 (merged as #594, released in
v0.6.0).Changes
Removed
ModelProviderRegistry.default, default-provider validators, andget_default_provider_name()from the engine registry.get_default_provider_name()helper.set_default/get_defaultservice methods, default-tracking in update/delete, the "Change default provider" menu option, and the "Default" column fromdata-designer config list.default_providerand per-itemconfigured_provider/effective_providerfields from agent introspection output in favor of a single explicit per-itemproviderfield.Changed
ModelConfig.providerrequired.ModelProviderRegistry.get_provider(name)to require a concrete provider name.resolve_model_provider_registry(providers)to dropdefault_provider_name.DataDesigner.__init__so it resolves providers without consulting YAML defaults.provideras required, and call out the upgrade impact for stalemodel_configs.yamlentries and agent context consumers.Compatibility
model_providers.yamlfiles with a legacydefault:key still load cleanly because bothModelProviderRegistryclasses inherit directly frompydantic.BaseModel, preserving pydantic v2's defaultextra="ignore"behavior. This invariant is now documented on the classes and covered by tests.model_configs.yamlentries that omitprovideror set it tonullmust be updated with an explicit provider name.Attention Areas
Reviewers should pay special attention to:
packages/data-designer-config/src/data_designer/config/models.py: public API change,ModelConfig.provideris now required.packages/data-designer-engine/src/data_designer/engine/model_provider.py: registry-level default routing is removed.packages/data-designer/src/data_designer/cli/utils/agent_introspection.py: agent context schema now reports onlyproviderper model alias item.Testing
PYTHONPATH=packages/data-designer-config/src:packages/data-designer-engine/src:packages/data-designer/src uv run --group dev pytest packages/data-designer/tests/cli/forms/test_model_builder.py packages/data-designer-engine/tests/engine/test_model_provider.py packages/data-designer/tests/cli/repositories/test_provider_repository.py packages/data-designer/tests/interface/test_data_designer.py packages/data-designer-config/tests/config/test_models.py packages/data-designer-config/tests/config/test_default_model_settings.py -q(162 passed before the final docs-wording amend)PYTHONPATH=packages/data-designer-config/src:packages/data-designer-engine/src:packages/data-designer/src uv run --group dev ruff check ...PYTHONPATH=packages/data-designer-config/src:packages/data-designer-engine/src:packages/data-designer/src uv run --group dev ruff format --check ...fern check: not run locally because thefernCLI is not installed in this environmentChecklist