refactor: remove ModelProviderRegistry.default and require ModelConfig.provider#625
refactor: remove ModelProviderRegistry.default and require ModelConfig.provider#625
Conversation
…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.
|
Docs preview: https://9e548b73.dd-docs-preview.pages.dev
|
Review: PR #625 —
|
Greptile SummaryFinal cleanup completing the #589 deprecation cycle:
|
| Filename | Overview |
|---|---|
| packages/data-designer-config/src/data_designer/config/models.py | Makes ModelConfig.provider a required str field; removes the _warn_on_implicit_provider validator and warn_at_caller import. Clean breaking-change completion of the #589 deprecation cycle. |
| packages/data-designer-engine/src/data_designer/engine/model_provider.py | Removes ModelProviderRegistry.default field, all three related validators, get_default_provider_name(), and the None-handling branch from get_provider(). resolve_model_provider_registry drops default_provider_name kwarg and simplifies to a single-line construction. |
| packages/data-designer/src/data_designer/cli/forms/model_builder.py | Replaces the provider=None fallback with a ValueError in build_config; error message is slightly misleading when multiple providers are configured but none selected in form_data. |
| packages/data-designer/src/data_designer/cli/repositories/provider_repository.py | Drops the default field from the CLI ModelProviderRegistry model and the deprecation-warning emission in load(); relies on pydantic v2 extra="ignore" to silently discard legacy default: keys from on-disk YAMLs. |
| packages/data-designer/src/data_designer/cli/services/provider_service.py | Removes set_default/get_default methods and the default-tracking logic from update/delete. The empty-registry sentinel in add() drops default=None cleanly. |
| packages/data-designer/src/data_designer/interface/data_designer.py | Removes get_default_provider_name import and call; collapses the two-branch model_providers initialization into a single call; drops the warnings.catch_warnings block used to suppress duplicate deprecation warnings. |
| packages/data-designer/src/data_designer/cli/utils/agent_introspection.py | Replaces the effective_provider/configured_provider/default_provider fields with a single provider field; simplifies provider usability check now that mc.provider is always a non-None str. |
| packages/data-designer/src/data_designer/cli/utils/agent_text_formatter.py | Removes default_provider header line from format_model_aliases_text and _format_model_aliases_context; simplifies _format_table by dropping the column_labels parameter. |
| packages/data-designer/src/data_designer/cli/controllers/provider_controller.py | Removes _handle_change_default handler and the conditional insertion of "change_default" in the menu; "exit" is now a fixed entry in the main options dict rather than appended last. |
Sequence Diagram
sequenceDiagram
participant User
participant DataDesigner
participant resolve_model_provider_registry
participant ModelProviderRegistry
participant ModelConfig
User->>DataDesigner: "__init__(model_providers=[...])"
DataDesigner->>resolve_model_provider_registry: (model_providers)
resolve_model_provider_registry->>ModelProviderRegistry: "ModelProviderRegistry(providers=model_providers)"
note over ModelProviderRegistry: No default field.<br/>Duplicate-name check only.
ModelProviderRegistry-->>DataDesigner: registry
User->>ModelConfig: "ModelConfig(alias=..., model=..., provider="nvidia")"
note over ModelConfig: provider is required str.<br/>ValidationError if missing.
ModelConfig-->>User: config
User->>DataDesigner: run(config)
DataDesigner->>ModelProviderRegistry: get_provider(config.provider)
note over ModelProviderRegistry: KeyError to UnknownProviderError<br/>if name not registered
ModelProviderRegistry-->>DataDesigner: ModelProvider
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/data-designer/src/data_designer/cli/forms/model_builder.py:294-299
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`."
)
```
Reviews (1): Last reviewed commit: "refactor: remove ModelProviderRegistry.d..." | 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.1 participant
📋 Summary
Final cleanup after the #589 deprecation cycle. Providers become a named list referenced by name end-to-end — no hidden registry default, no YAML
default:leaking intoDataDesigner.__init__(#588), no ambiguity about which provider wins. Net change is +109 / -831 across 31 files (large deletion = collapsing a deprecated surface).🔗 Related Issue
Closes #590. Depends on #588 (merged as #591) and #589 (merged as #594).
🔄 Changes
🗑️ Removed
ModelProviderRegistry.defaultfield,check_implicit_default/check_default_exists/_warn_on_explicit_defaultvalidators, andget_default_provider_name()method (packages/data-designer-engine/src/data_designer/engine/model_provider.py).get_default_provider_name()helper (packages/data-designer-config/src/data_designer/config/default_model_settings.py).defaultfield on the repository'sModelProviderRegistry(provider_repository.py),set_default/get_default/ default-tracking inupdate/delete(provider_service.py), and the "Change default provider" interactive menu option (provider_controller.py).data-designer config listoutput (commands/list.py).default_provider/effective_providerfields from agent introspection output (agent_introspection.py,agent_text_formatter.py).🔧 Changed
ModelConfig.provider:str | None = None→str(required) (models.py).ModelProviderRegistry.get_provider(name)now requires astr;Nonehandling removed.resolve_model_provider_registry(providers)drops thedefault_provider_namekwarg.DataDesigner.__init__no longer readsget_default_provider_name()or threads a YAML default through registry construction (data_designer.py).ModelFormBuilder.build_configraisesValueErrorinstead of constructingModelConfig(provider=None)when no providers are available (model_builder.py).docs/concepts/models/updated to drop deprecation warnings and the "Change default provider" workflow.README.mdandscripts/benchmarks/benchmark_engine_v2.pyupdated to match the new signatures.🧪 Tests
test_models.py,test_default_model_settings.py,test_model_provider.py,test_provider_repository.py,test_provider_service.py,test_provider_controller.py, andtest_data_designer.pywith explicit-required-field tests.test_registry_ignores_legacy_default_keyandtest_load_silently_ignores_legacy_default_keyto lock in the migration path: pydantic v2'sextra="ignore"default silently drops legacydefault:keys from existing YAMLs.test_init_no_user_providers_loads_from_yamlto cover the now-clean YAML fallback path inDataDesigner.__init__.🔍 Attention Areas
packages/data-designer-config/src/data_designer/config/models.py— Public API change:ModelConfig.provideris now required. Anyone constructingModelConfig(alias=..., model=...)withoutprovider=gets aValidationError. Migration is one-line per call site.packages/data-designer-engine/src/data_designer/engine/model_provider.py— Public API change:get_provider(name: str)no longer acceptsNone;resolve_model_provider_registry()no longer acceptsdefault_provider_name.default:— relies onpydantic.BaseModel's defaultextra="ignore". BothModelProviderRegistryclasses inherit fromBaseModeldirectly (notConfigBase, which overrides toextra="forbid"), so old configs load without error. Locked in bytest_registry_ignores_legacy_default_keyandtest_load_silently_ignores_legacy_default_key.🧪 Testing
make testpasses (3229 tests across config + engine + interface)make check-all-fixpasses (ruff lint + format)✅ Checklist