fix(models): address PR #594 review feedback

Greptile P1: ProviderRepository.load emitted its DeprecationWarning
inside a `try/except Exception` block. Under
`filterwarnings("error", DeprecationWarning)` the warn would raise,
the except would swallow it, and `load()` would silently return None
(losing the registry). Move the warn outside the catch-all so the
strict-warning path no longer drops valid configs.

Greptile P2 / johnnygreco: `_warn_on_implicit_provider` and
`_warn_on_explicit_default` use `stacklevel=2`, which lands inside
pydantic v2's validator dispatch rather than at the user's
`ModelConfig(...)` / `ModelProviderRegistry(...)` call. That broke
both attribution (the source line was unhelpful) and Python's
once-per-location dedup (every call collapsed to the same
pydantic-internal key, suppressing all but the first warning).
Introduce `data_designer.config.utils.warning_helpers.warn_at_caller`,
which walks past the helper, validator, and any pydantic frames to
find the user's call site and emits via `warnings.warn_explicit` with
the user frame's `__warningregistry__`. Keeps attribution accurate
and dedup keyed on the user's (filename, lineno).

johnnygreco: align the `provider_repository.py` warning copy with the
sibling site in `default_model_settings.py` ("specify provider=
explicitly on each ModelConfig instead") so both YAML-default warning
sites give the same migration instruction. The previous wording
pointed users at "ModelConfig entries" inside `model_providers.yaml`,
where ModelConfig entries don't actually live.

johnnygreco: dedup the cascade in `DataDesigner.__init__`. With
`model_providers=None` and a YAML `default:`, the user previously saw
two DeprecationWarnings for the same root cause —
`get_default_provider_name()` warns about the YAML key, then
`resolve_model_provider_registry(...)` re-warns from
`_warn_on_explicit_default`. Suppress the registry-level duplicate in
the YAML-fallback branch via `warnings.catch_warnings()` so users see
exactly one warning per user action.

johnnygreco: tighten `_warn_on_explicit_default` to fire only when
`default is not None`. Passing `default=None` explicitly is
semantically equivalent to omitting it (caller is opting *out* of a
registry-level default), and shouldn't trigger the deprecation
nudge.

johnnygreco: add a `model_validate({...})` regression test for
`ModelConfig` so the deserialization path (legacy on-disk configs)
is pinned alongside the construction path.

Tests:
- Update `test_load_exists` and `test_save` to omit `default=` so the
  roundtrip stops exercising the deprecated YAML-default path
  unguarded (Greptile note).
- Wrap `test_resolve_model_provider_registry_with_explicit_default`,
  `test_get_provider`, and
  `test_init_user_supplied_providers_preserve_first_wins_over_yaml_default`
  in `pytest.warns` so the suite stays green under
  `-W error::DeprecationWarning` (Greptile note).
- Add `test_explicit_default_none_does_not_emit_deprecation_warning`
  to pin the tightened predicate.
- Add `test_init_yaml_default_emits_single_deprecation_warning` to
  pin the cascade-dedup behavior.

Refs #589

Made-with: Cursor

Author: nabinchha

packages/data-designer-config/src/data_designer/config/models.py

@@ -5,7 +5,6 @@
 
 import json
 import logging
-import warnings
 from abc import ABC, abstractmethod
 from enum import Enum
 from pathlib import Path
@@ -32,6 +31,7 @@
     load_image_path_to_base64,
 )
 from data_designer.config.utils.io_helpers import smart_load_yaml
+from data_designer.config.utils.warning_helpers import warn_at_caller
 
 logger = logging.getLogger(__name__)
 
@@ -542,12 +542,17 @@ def _convert_inference_parameters(cls, value: Any) -> Any:
     @model_validator(mode="after")
     def _warn_on_implicit_provider(self) -> Self:
         if self.provider is None:
-            msg = (
+            # Use ``warn_at_caller`` so the warning is attributed to the user's
+            # ``ModelConfig(...)`` / ``model_validate(...)`` call rather than a
+            # pydantic-internal frame. Without this, every call dedupes to the
+            # same pydantic line and only the first emission is shown. See
+            # PR #594 review.
+            warn_at_caller(
                 f"ModelConfig.provider=None is deprecated and will be required in a future release. "
                 f"Specify provider= explicitly on ModelConfig(alias={self.alias!r}, ...). "
-                "See issue #589."
+                "See issue #589.",
+                DeprecationWarning,
             )
-            warnings.warn(msg, DeprecationWarning, stacklevel=2)
         return self
 
 

packages/data-designer-config/src/data_designer/config/utils/warning_helpers.py

@@ -0,0 +1,67 @@
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+"""Helpers for emitting warnings that attribute correctly to user code.
+
+Pydantic v2 dispatches ``@model_validator`` / ``@field_validator`` callbacks
+through several internal frames. ``warnings.warn(stacklevel=N)`` from inside a
+validator therefore tends to land inside pydantic's machinery rather than at
+the user's ``ModelConfig(...)`` / ``ModelProviderRegistry(...)`` call site.
+
+That breaks two things:
+
+1. Attribution — the displayed source location is unhelpful.
+2. Deduplication — Python's default once-per-location dedup key is
+   ``(category, module, lineno)``. When every call resolves to the same
+   pydantic-internal line, every warning after the first is silently
+   suppressed regardless of which user file triggered it.
+
+``warn_at_caller`` walks the stack to the first frame outside pydantic (and
+outside this helper / the calling validator) and uses
+``warnings.warn_explicit`` to attribute the warning there.
+"""
+
+from __future__ import annotations
+
+import sys
+import warnings
+
+
+def warn_at_caller(message: str, category: type[Warning]) -> None:
+    """Emit ``message`` attributed to the first non-pydantic frame above the caller.
+
+    Intended to be invoked from inside a pydantic validator. The walk skips this
+    helper's own frame and the calling validator's frame, then walks past any
+    pydantic-internal frames until it finds the user's call site.
+
+    The user frame's ``__warningregistry__`` is passed to
+    ``warnings.warn_explicit`` so Python's built-in once-per-location dedup keys
+    on the *user's* (filename, lineno) rather than an internal pydantic frame.
+    That matches how ``warnings.warn`` would dedup if ``stacklevel`` could
+    correctly point at the user.
+
+    We deliberately do *not* pass ``module_globals`` — it's only used for
+    ``linecache`` source-line display, and for scripts run with ``python -c``
+    (where the user frame's ``__loader__`` is ``BuiltinImporter`` for
+    ``__main__``) the lookup raises ``ImportError("'__main__' is not a built-in
+    module")``. Skipping ``module_globals`` keeps the warning path robust at
+    the cost of an empty source line in the formatted output.
+    """
+    # Skip frame 0 (warn_at_caller) and frame 1 (the validator that called us).
+    frame = sys._getframe(2) if hasattr(sys, "_getframe") else None
+    while frame is not None:
+        module_name = frame.f_globals.get("__name__", "")
+        if not module_name.startswith("pydantic"):
+            warnings.warn_explicit(
+                message,
+                category,
+                frame.f_code.co_filename,
+                frame.f_lineno,
+                module=module_name,
+                registry=frame.f_globals.setdefault("__warningregistry__", {}),
+            )
+            return
+        frame = frame.f_back
+
+    # Fallback: never escaped pydantic frames (or no frame access). Use stacklevel.
+    warnings.warn(message, category, stacklevel=3)

packages/data-designer-config/tests/config/test_models.py

@@ -494,6 +494,17 @@ def test_model_config_provider_none_emits_deprecation_warning():
         ModelConfig(alias="legacy", model="legacy-model", provider=None)
 
 
+def test_model_config_provider_none_via_model_validate_emits_deprecation_warning():
+    """Regression for #589 / PR #594 review: deserialising legacy on-disk configs
+    via ``ModelConfig.model_validate(...)`` must surface the same
+    ``DeprecationWarning`` as direct construction. Both paths funnel through
+    the same validator today, so this pin protects against a future refactor
+    that, e.g., only runs the validator on construction and not on revalidation.
+    """
+    with pytest.warns(DeprecationWarning, match="ModelConfig.provider=None is deprecated"):
+        ModelConfig.model_validate({"alias": "legacy", "model": "legacy-model"})
+
+
 def test_model_config_with_provider_does_not_warn():
     """Pin the post-deprecation happy path: specifying ``provider=`` must not
     emit any deprecation warning.

packages/data-designer-engine/src/data_designer/engine/model_provider.py

@@ -3,14 +3,14 @@
 
 from __future__ import annotations
 
-import warnings
 from functools import cached_property
 
 from pydantic import BaseModel, field_validator, model_validator
 from typing_extensions import Self
 
 from data_designer.config.mcp import MCPProviderT
 from data_designer.config.models import ModelProvider
+from data_designer.config.utils.warning_helpers import warn_at_caller
 from data_designer.engine.errors import NoModelProvidersError, UnknownProviderError
 
 
@@ -56,17 +56,21 @@ def check_default_exists(self) -> Self:
 
     @model_validator(mode="after")
     def _warn_on_explicit_default(self) -> Self:
-        # Fires only when the caller actually passed ``default=`` (not when it's
-        # left at the field default of None). ``resolve_model_provider_registry``
-        # avoids passing ``default=`` in the single-provider case so common
-        # construction paths stay quiet. See issue #589.
-        if "default" in self.model_fields_set:
-            warnings.warn(
+        # Fires only when the caller actually passed a non-None ``default=``.
+        # The ``model_fields_set`` guard distinguishes "caller opted into the
+        # deprecated field" from "field at its default value of None", and the
+        # ``self.default is not None`` clause additionally lets callers
+        # explicitly opt *out* via ``default=None`` without tripping the
+        # warning. ``resolve_model_provider_registry`` avoids passing
+        # ``default=`` in the single-provider case so common construction paths
+        # stay quiet. ``warn_at_caller`` keeps attribution and dedup correct
+        # under pydantic's validator dispatch. See issue #589 / PR #594 review.
+        if "default" in self.model_fields_set and self.default is not None:
+            warn_at_caller(
                 "ModelProviderRegistry.default is deprecated and will be removed in a "
                 "future release. Specify provider= explicitly on each ModelConfig "
                 "instead of relying on a registry-level default. See issue #589.",
                 DeprecationWarning,
-                stacklevel=2,
             )
         return self
 

packages/data-designer-engine/tests/engine/test_model_provider.py

@@ -58,10 +58,14 @@ def test_no_duplicate_provider_names(stub_foo_provider: ModelProvider):
 
 
 def test_get_provider(stub_foo_provider: ModelProvider, stub_bar_provider: ModelProvider):
-    registry = ModelProviderRegistry(
-        providers=[stub_foo_provider, stub_bar_provider],
-        default="foo",
-    )
+    # Multi-provider construction with an explicit default exercises the #589
+    # deprecation path; wrap so this test stays green if the project ever runs
+    # with ``-W error::DeprecationWarning``.
+    with pytest.warns(DeprecationWarning, match="ModelProviderRegistry.default is deprecated"):
+        registry = ModelProviderRegistry(
+            providers=[stub_foo_provider, stub_bar_provider],
+            default="foo",
+        )
 
     assert registry.get_provider(None) == stub_foo_provider
     assert registry.get_provider("foo") == stub_foo_provider
@@ -82,8 +86,15 @@ def test_resolve_model_provider_registry(stub_foo_provider: ModelProvider) -> No
 def test_resolve_model_provider_registry_with_explicit_default(
     stub_foo_provider: ModelProvider, stub_bar_provider: ModelProvider
 ) -> None:
-    """Test resolve_model_provider_registry with explicit default."""
-    registry = resolve_model_provider_registry([stub_foo_provider, stub_bar_provider], default_provider_name="bar")
+    """Test resolve_model_provider_registry with explicit default.
+
+    The multi-provider/explicit-default path is the deprecated one (see #589),
+    so the construction emits a ``DeprecationWarning``. Wrap the call in
+    ``pytest.warns`` so this test stays green if the project ever runs under
+    ``-W error::DeprecationWarning``.
+    """
+    with pytest.warns(DeprecationWarning, match="ModelProviderRegistry.default is deprecated"):
+        registry = resolve_model_provider_registry([stub_foo_provider, stub_bar_provider], default_provider_name="bar")
 
     assert registry.get_default_provider_name() == "bar"
 

packages/data-designer/src/data_designer/cli/repositories/provider_repository.py

@@ -36,14 +36,24 @@ def load(self) -> ModelProviderRegistry | None:
 
         try:
             config_dict = load_config_file(self.config_file)
-            if config_dict.get("default") is not None:
-                warnings.warn(
-                    f"The 'default:' key in {self.config_file} is deprecated and will be "
-                    "removed in a future release. Remove it and refer to providers by name "
-                    "in your ModelConfig entries. See issue #589.",
-                    DeprecationWarning,
-                    stacklevel=2,
-                )
+        except Exception:
+            return None
+
+        # Emit the deprecation warning *outside* the validation try/except below.
+        # ``DeprecationWarning`` is an ``Exception`` subclass, so under
+        # ``filterwarnings("error", DeprecationWarning)`` a warn raised inside
+        # the catch-all would be silently swallowed and ``load`` would drop the
+        # registry. See PR #594 review.
+        if config_dict.get("default") is not None:
+            warnings.warn(
+                f"The 'default:' key in {self.config_file} is deprecated and will "
+                "be removed in a future release. Remove it and specify provider= "
+                "explicitly on each ModelConfig instead. See issue #589.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+
+        try:
             return ModelProviderRegistry.model_validate(config_dict)
         except Exception:
             return None

packages/data-designer/src/data_designer/interface/data_designer.py

@@ -4,6 +4,7 @@
 from __future__ import annotations
 
 import logging
+import warnings
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -163,7 +164,21 @@ def __init__(
             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, default_provider_name)
+        # When the YAML carries a default, ``get_default_provider_name`` already
+        # nudged the user with a ``DeprecationWarning``. Building the registry
+        # below would re-fire ``ModelProviderRegistry._warn_on_explicit_default``
+        # for the same root cause, so suppress that second warning. See PR #594
+        # review.
+        with warnings.catch_warnings():
+            if default_provider_name is not None:
+                warnings.filterwarnings(
+                    "ignore",
+                    message="ModelProviderRegistry.default is deprecated",
+                    category=DeprecationWarning,
+                )
+            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

packages/data-designer/tests/cli/repositories/test_provider_repository.py

@@ -23,19 +23,25 @@ def test_load_does_not_exist():
 
 
 def test_load_exists(tmp_path: Path, stub_model_providers: list[ModelProvider]):
+    # Roundtrip test for the load/save cycle. We deliberately leave ``default``
+    # unset so this test does not exercise the deprecated YAML ``default:`` path
+    # — that path is covered by ``test_load_with_yaml_default_emits_deprecation_warning``
+    # below. See issue #589.
     providers_file_path = tmp_path / MODEL_PROVIDERS_FILE_NAME
     save_config_file(
         providers_file_path,
-        ModelProviderRegistry(providers=stub_model_providers, default=stub_model_providers[0].name).model_dump(),
+        ModelProviderRegistry(providers=stub_model_providers).model_dump(exclude_none=True),
     )
     repository = ProviderRepository(tmp_path)
     assert repository.load() is not None
     assert repository.load().providers == stub_model_providers
 
 
 def test_save(tmp_path: Path, stub_model_providers: list[ModelProvider]):
+    # As above, leave ``default`` unset so the roundtrip stays clear of the
+    # YAML-default deprecation. See issue #589.
     repository = ProviderRepository(tmp_path)
-    repository.save(ModelProviderRegistry(providers=stub_model_providers, default=stub_model_providers[0].name))
+    repository.save(ModelProviderRegistry(providers=stub_model_providers))
     assert repository.load() is not None
     assert repository.load().providers == stub_model_providers
 

packages/data-designer/tests/interface/test_data_designer.py

@@ -6,6 +6,7 @@
 import contextlib
 import json
 import logging
+import warnings
 from datetime import datetime
 from pathlib import Path
 from typing import Any
@@ -464,7 +465,13 @@ def test_init_user_supplied_providers_preserve_first_wins_over_yaml_default(
         ),
     ]
 
-    with patch.object(dd_mod, "get_default_provider_name", return_value="second-provider"):
+    # Multi-provider construction (user-supplied list of length > 1) still
+    # passes ``default=`` to ``ModelProviderRegistry`` — that's the deprecated
+    # path under #589 — so the registry-level deprecation fires here.
+    with (
+        patch.object(dd_mod, "get_default_provider_name", return_value="second-provider"),
+        pytest.warns(DeprecationWarning, match="ModelProviderRegistry.default is deprecated"),
+    ):
         data_designer = DataDesigner(
             artifact_path=stub_artifact_path,
             model_providers=user_providers,
@@ -513,6 +520,61 @@ def test_init_no_user_providers_uses_yaml_default(
     assert data_designer.model_provider_registry.get_default_provider_name() == "yaml-second"
 
 
+def test_init_yaml_default_emits_single_deprecation_warning(
+    stub_artifact_path: Path,
+    stub_managed_assets_path: Path,
+) -> None:
+    """Regression for PR #594 review: when ``DataDesigner()`` falls back to the
+    YAML's ``providers:`` and ``default:``, the user should see a single
+    ``DeprecationWarning`` (the YAML one) rather than a duplicate cascade where
+    ``ModelProviderRegistry._warn_on_explicit_default`` also fires for the same
+    root cause. See issue #589.
+    """
+    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 warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter("always", DeprecationWarning)
+        with (
+            patch.object(dd_mod, "get_default_providers", return_value=yaml_providers),
+            patch.object(dd_mod, "get_default_provider_name") as mock_get_default,
+        ):
+            mock_get_default.side_effect = lambda: (
+                warnings.warn(
+                    "The 'default:' key in /fake/path is deprecated and will "
+                    "be removed in a future release. Remove it and specify provider= "
+                    "explicitly on each ModelConfig instead. See issue #589.",
+                    DeprecationWarning,
+                    stacklevel=2,
+                )
+                or "yaml-second"
+            )
+            DataDesigner(
+                artifact_path=stub_artifact_path,
+                secret_resolver=PlaintextResolver(),
+                managed_assets_path=stub_managed_assets_path,
+            )
+
+    deprecation_messages = [str(w.message) for w in caught if issubclass(w.category, DeprecationWarning)]
+    yaml_default_warnings = [m for m in deprecation_messages if "'default:' key" in m]
+    registry_default_warnings = [m for m in deprecation_messages if "ModelProviderRegistry.default is deprecated" in m]
+    assert len(yaml_default_warnings) == 1, deprecation_messages
+    assert registry_default_warnings == [], (
+        "Registry-level deprecation should be suppressed in the YAML-fallback path "
+        "to avoid two warnings for the same root cause."
+    )
+
+
 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)