From 5bfc9a9887f28ae0bb53a2af829c5d9269bf2d81 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 24 Jun 2026 15:37:41 +0000 Subject: [PATCH 01/19] refactor(config): centralize sparse config patch validation Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/cli/utils.py | 9 +- .../config/autoconfig.py | 10 +- .../config/parameters.py | 110 +++++++++++------- .../configurator/parameters.py | 50 +++++--- .../sdk/config_builder.py | 89 +++++++------- src/nemo_safe_synthesizer/utils.py | 8 +- tests/config/test_parameters.py | 64 +++++++++- tests/sdk/test_config_builder.py | 43 +++++++ 8 files changed, 276 insertions(+), 107 deletions(-) create mode 100644 tests/sdk/test_config_builder.py diff --git a/src/nemo_safe_synthesizer/cli/utils.py b/src/nemo_safe_synthesizer/cli/utils.py index 4d6908cdf..6084537ca 100644 --- a/src/nemo_safe_synthesizer/cli/utils.py +++ b/src/nemo_safe_synthesizer/cli/utils.py @@ -24,6 +24,7 @@ from pydantic import ValidationError from ..config import SafeSynthesizerParameters +from ..config.parameters import ConfigPatch from ..defaults import DEFAULT_ARTIFACTS_PATH from ..observability import configure_logging_from_workdir, get_logger, initialize_observability from ..utils import merge_dicts @@ -431,7 +432,7 @@ def _initialize_logging_for_cli_from_settings( return run_logger -def merge_overrides(config_path: str | Path | None, overrides: dict) -> SafeSynthesizerParameters: +def merge_overrides(config_path: str | Path | None, overrides: ConfigPatch) -> SafeSynthesizerParameters: """Merge overrides into a SafeSynthesizerParameters object. If config_path is None, use the overrides to create a new SafeSynthesizerParameters object. @@ -446,11 +447,9 @@ def merge_overrides(config_path: str | Path | None, overrides: dict) -> SafeSynt """ try: if config_path is None: - my_config = SafeSynthesizerParameters.model_validate(overrides) + my_config = SafeSynthesizerParameters.from_config_patch(overrides) else: - file_config = SafeSynthesizerParameters.from_yaml(config_path).model_dump(exclude_unset=True) - params = merge_dicts(file_config, overrides) - my_config = SafeSynthesizerParameters.model_validate(params) + my_config = SafeSynthesizerParameters.from_yaml(config_path).with_config_patch(overrides) except ValidationError as e: click.echo(f"{config_path} is invalid:\n{e}") sys.exit(1) diff --git a/src/nemo_safe_synthesizer/config/autoconfig.py b/src/nemo_safe_synthesizer/config/autoconfig.py index cab8c3877..fb7770748 100644 --- a/src/nemo_safe_synthesizer/config/autoconfig.py +++ b/src/nemo_safe_synthesizer/config/autoconfig.py @@ -20,8 +20,7 @@ from ..defaults import DEFAULT_MAX_SEQ_LENGTH, MAX_ROPE_SCALING_FACTOR from ..llm.metadata import ModelMetadata from ..observability import get_logger -from ..utils import merge_dicts -from .parameters import SafeSynthesizerParameters +from .parameters import ConfigPatch, SafeSynthesizerParameters from .types import AUTO_STR if TYPE_CHECKING: @@ -304,14 +303,13 @@ def _build_updated_params( Returns: The validated SafeSynthesizerParameters. """ - new_params = { + new_params: ConfigPatch = { "training": training_params, "data": data_params, "privacy": privacy_params, } - updated_params = merge_dicts(self._config.model_dump(exclude_unset=True), new_params) - logger.debug(f"params to update: {updated_params}") - my_config = SafeSynthesizerParameters.model_validate(updated_params) + logger.debug(f"params to update: {new_params}") + my_config = self._config.with_config_patch(new_params) logger.debug(f"auto-updated config: {my_config.model_dump(exclude_unset=True)}") return my_config diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index ceee20d14..dff90c3e3 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -5,9 +5,10 @@ import warnings from collections.abc import Mapping -from typing import Any, Self +from typing import Self, TypeAlias from pydantic import BaseModel, Field, model_validator +from typing_extensions import override from ..configurator.parameters import Parameters from ..errors import ParameterError @@ -24,13 +25,16 @@ from .training import TrainingHyperparams from .types import AUTO_STR -__all__ = ["SafeSynthesizerParameters"] +ConfigPatch: TypeAlias = Mapping[str, object] +_SectionPatch: TypeAlias = dict[str, object] + +__all__ = ["ConfigPatch", "SafeSynthesizerParameters"] logger = get_logger(__name__) -def _collect_set_fields(model: BaseModel) -> dict[str, Any]: +def _collect_set_fields(model: BaseModel) -> _SectionPatch: """Recursively collect a model's explicitly-set fields as a nested dict. Unlike ``model_dump(exclude_unset=True)``, nested models are always @@ -39,7 +43,7 @@ def _collect_set_fields(model: BaseModel) -> dict[str, Any]: ``cfg.generation.validation.foo = True``) are captured. A nested model is included only when it has at least one set field of its own. """ - overrides: dict[str, Any] = {} + overrides: _SectionPatch = {} for name in type(model).model_fields: value = getattr(model, name) if isinstance(value, BaseModel): @@ -66,21 +70,23 @@ def _overlay_set_fields(saved: Parameters, runtime: Parameters) -> Parameters: return saved.model_validate(merge_dicts(saved.model_dump(), overrides)) -def _section_values(kwargs: dict[str, Any], section: str) -> dict[str, Any]: - """Return values for a nested section merged with flat compatibility kwargs.""" - section_value = kwargs.get(section) - match section_value: - case BaseModel() as model: - section_data = model.model_dump() - case Mapping() as mapping: - section_data = dict(mapping) - case None: - section_data = {} - case _: - section_data = {section: section_value} +def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object) -> None: + """Assign ``value`` into ``target`` at a dotted config path.""" + head, *tail = path + if not tail: + target[head] = value + return - flat_values = {key: value for key, value in kwargs.items() if key != section} - return section_data | flat_values + next_value = target.get(head) + if next_value is None: + nested: dict[str, object] = {} + target[head] = nested + elif isinstance(next_value, dict): + nested = {str(key): item for key, item in next_value.items()} + target[head] = nested + else: + raise ParameterError(f"Cannot assign nested parameter path {'.'.join(path)!r}; {head!r} is already set.") + _assign_path(nested, tuple(tail), value) class SafeSynthesizerParameters(Parameters): @@ -210,6 +216,7 @@ def check_timeseries_group_column(self) -> Self: return self @classmethod + @override def from_params(cls, **kwargs) -> "SafeSynthesizerParameters": """Convert singular, flat parameters to nested structure. @@ -232,28 +239,53 @@ def from_params(cls, **kwargs) -> "SafeSynthesizerParameters": >>> from nemo_safe_synthesizer.config import SafeSynthesizerParameters >>> SafeSynthesizerParameters.from_params(structured_generation={"enabled": True}) """ - thp = TrainingHyperparams.model_validate(_section_values(kwargs, "training")) - gp = GenerateParameters.model_validate(_section_values(kwargs, "generation")) - ep = EvaluationParameters.model_validate(_section_values(kwargs, "evaluation")) - pp = DifferentialPrivacyHyperparams.model_validate(_section_values(kwargs, "privacy")) - dp = DataParameters.model_validate(_section_values(kwargs, "data")) - tsp = TimeSeriesParameters.model_validate(_section_values(kwargs, "time_series")) - - extra: dict[str, Any] = { - "training": thp, - "generation": gp, - "evaluation": ep, - "privacy": pp, - "data": dp, - "time_series": tsp, + section_defaults: dict[str, Parameters] = { + "training": TrainingHyperparams(), + "generation": GenerateParameters(), + "evaluation": EvaluationParameters(), + "privacy": DifferentialPrivacyHyperparams(), + "data": DataParameters(), + "time_series": TimeSeriesParameters(), + "preflight": PreflightParameters(), } - if "replace_pii" in kwargs: - extra["replace_pii"] = kwargs["replace_pii"] - if "preflight" in kwargs: - extra["preflight"] = kwargs["preflight"] - if "emit_telemetry" in kwargs: - extra["emit_telemetry"] = kwargs["emit_telemetry"] - return cls(**extra) + top_level_fields = set(cls.model_fields) + field_index: dict[str, list[tuple[str, ...]]] = {} + for section_name, section in section_defaults.items(): + for path, _ in section._iter_field_paths((section_name,)): + field_index.setdefault(path[-1], []).append(path) + + patch: dict[str, object] = {} + for name, value in kwargs.items(): + if "." in name: + _assign_path(patch, tuple(name.split(".")), value) + continue + if name in top_level_fields: + patch[name] = value + continue + matches = field_index.get(name, []) + if not matches: + raise ParameterError(f"Unknown parameter name {name!r}.") + if len(matches) > 1: + candidates = ", ".join(".".join(path) for path in matches) + raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") + _assign_path(patch, matches[0], value) + + return cls.model_validate(patch) + + @classmethod + def from_config_patch(cls, patch: ConfigPatch) -> Self: + """Validate a sparse top-level config patch as a full configuration.""" + return cls.model_validate(patch) + + def with_config_patch(self, patch: ConfigPatch) -> Self: + """Apply a sparse top-level config patch and revalidate the result. + + Only fields explicitly set on ``self`` are carried into the merge before + applying ``patch``. This preserves file/CLI precedence while keeping + default values implicit for future ``exclude_unset`` dumps. + """ + params = merge_dicts(self.model_dump(exclude_unset=True), patch) + return type(self).model_validate(params) def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSynthesizerParameters": """Apply resume-time generation/evaluation/telemetry overrides onto a copy of self. diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index 5e5211666..e559c0b1f 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -31,6 +31,7 @@ from ..config.base import ( pydantic_model_config, ) +from ..errors import ParameterError from .parameter import ( DataT, ) @@ -38,6 +39,7 @@ __all__ = ["Parameters"] PathT = str | Path +_MISSING = object() class Parameters(BaseModel, metaclass=ABCMeta): @@ -109,6 +111,24 @@ def _iter_parameters(self, recursive: bool = True) -> Generator[Mapping[str, Any for pg in param_groups: yield from pg._iter_parameters(recursive=True) + def _iter_field_paths(self, prefix: tuple[str, ...] = ()) -> Iterator[tuple[tuple[str, ...], Any]]: + """Yield every field path and value in this parameter tree.""" + for name in type(self).model_fields: + value = getattr(self, name) + path = (*prefix, name) + yield path, value + if isinstance(value, Parameters): + yield from value._iter_field_paths(path) + + def _get_field_path(self, path: tuple[str, ...]) -> object: + """Resolve an explicit field path, returning ``_MISSING`` when absent.""" + value: object = self + for part in path: + if not isinstance(value, Parameters) or part not in type(value).model_fields: + return _MISSING + value = getattr(value, part) + return value + def __iter__(self) -> Iterator[Mapping[str, Any]]: # ty: ignore[invalid-method-override] -- intentionally overrides pydantic BaseModel.__iter__ with parameter-group semantics """Iterate over all parameters, recursing into nested groups.""" return self._iter_parameters(recursive=True) @@ -116,7 +136,9 @@ def __iter__(self) -> Iterator[Mapping[str, Any]]: # ty: ignore[invalid-method- def get(self, name: str, default: Any = None) -> DataT | Any | None: """Look up a parameter or sub-group by name across the full tree. - Checks direct attributes first, then walks nested groups recursively. + Explicit dotted paths such as ``"generation.validation.foo"`` resolve + directly. Bare names are accepted only when they map to exactly one + field in the parameter tree. Args: name: Field name to search for. @@ -125,12 +147,17 @@ def get(self, name: str, default: Any = None) -> DataT | Any | None: Returns: The parameter value or sub-group if found, otherwise ``default``. """ - if (group := getattr(self, name, None)) is not None: - return group - for param in self._iter_parameters(recursive=True): - if name in param: - return param.get(name) - return default + if "." in name: + value = self._get_field_path(tuple(name.split("."))) + return default if value is _MISSING else value + + matches = [(path, value) for path, value in self._iter_field_paths() if path[-1] == name] + if not matches: + return default + if len(matches) > 1: + candidates = ", ".join(".".join(path) for path, _ in matches) + raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") + return matches[0][1] def has(self, name: str) -> bool: """Check whether ``name`` exists anywhere in the parameter tree. @@ -144,12 +171,9 @@ def has(self, name: str) -> bool: Returns: ``True`` if the parameter or sub-group exists. """ - if getattr(self, name, None) is not None: - return True - for param in self._iter_parameters(recursive=True): - if name in param: - return True - return False + if "." in name: + return self._get_field_path(tuple(name.split("."))) is not _MISSING + return any(path[-1] == name for path, _ in self._iter_field_paths()) @classmethod def from_yaml_str(cls, raw: str) -> Self: diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index 8f5cd668f..d6f124f29 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -6,7 +6,7 @@ from __future__ import annotations from collections.abc import Mapping -from typing import Any, Self, TypeAlias, TypeVar +from typing import Self, TypeAlias, TypeVar, overload import pandas as pd from pydantic import BaseModel @@ -23,14 +23,10 @@ ) from ..observability import get_logger from ..telemetry import _telemetry_enabled -from ..utils import merge_dicts logger = get_logger(__name__) -KT = TypeVar("KT") -VT = TypeVar("VT") - NSSParameters = ( DataParameters | EvaluationParameters @@ -44,7 +40,8 @@ ParamT = TypeVar("ParamT", bound=BaseModel) DataSource = pd.DataFrame | str -ParamDict: TypeAlias = dict[str, str | int | float | bool | None | list[Any] | Mapping[KT, VT]] +RawConfig: TypeAlias = Mapping[str, object] +ParamDict: TypeAlias = RawConfig class ConfigBuilder(object): @@ -57,8 +54,8 @@ class ConfigBuilder(object): ``SafeSynthesizerParameters``. Each ``with_*`` method accepts an optional typed config object or - a plain dict, plus ``**kwargs`` overrides. ``kwargs`` always take - precedence over fields in the config/dict. All ``with_*`` methods + a raw mapping, plus ``**kwargs`` overrides. ``kwargs`` always take + precedence over fields in the config/mapping. All ``with_*`` methods return ``Self`` so subclasses preserve their concrete type through fluent chains. @@ -102,14 +99,23 @@ def __init__(self, config: SafeSynthesizerParameters | None = None) -> None: "_time_series_config", ] - def _resolve_config(self, values: ParamDict | NSSParameters | None, cls: type[ParamT], **kwargs) -> ParamT: + @overload + def _resolve_config(self, values: ParamT, cls: type[ParamT], **kwargs: object) -> ParamT: ... + + @overload + def _resolve_config(self, values: RawConfig, cls: type[ParamT], **kwargs: object) -> ParamT: ... + + @overload + def _resolve_config(self, values: None, cls: type[ParamT], **kwargs: object) -> ParamT: ... + + def _resolve_config(self, values: object, cls: type[ParamT], **kwargs: object) -> ParamT: """Resolve configuration from various input types. Precedence: ``kwargs`` override ``values``; ``values`` override model defaults. Args: - values: Existing config, a raw dict, or ``None`` for + values: Existing config, a raw mapping, or ``None`` for defaults-only. cls: The Pydantic model class to validate against. **kwargs: Field-level overrides applied on top. @@ -117,15 +123,19 @@ def _resolve_config(self, values: ParamDict | NSSParameters | None, cls: type[Pa Returns: A validated config instance of type ``cls``. """ - overrides = kwargs match values: - case BaseModel() as model: - data = model.model_dump() - return cls.model_validate(merge_dicts(data, overrides)) - case dict() as d: - return cls.model_validate(merge_dicts(d, overrides)) case None: - return cls.model_validate(overrides) + return cls.model_validate(kwargs) + case BaseModel() as model: + if not isinstance(model, cls): + raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") + raw_values = model.model_dump() + raw_values.update(kwargs) + return cls.model_validate(raw_values) + case Mapping() as mapping: + raw_values = dict(mapping) + raw_values.update(kwargs) + return cls.model_validate(raw_values) case _: raise TypeError(f"Unsupported config type: {type(values)}") @@ -141,11 +151,11 @@ def with_data_source(self, df_source: DataSource) -> Self: self._data_source = df_source return self - def with_data(self, config: DataParameters | ParamDict | None = None, **kwargs) -> Self: + def with_data(self, config: DataParameters | RawConfig | None = None, **kwargs: object) -> Self: """Configure data processing settings. Args: - config: Data configuration object or dict. + config: Data configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``holdout_size``). Returns: @@ -154,11 +164,11 @@ def with_data(self, config: DataParameters | ParamDict | None = None, **kwargs) self._data_config = self._resolve_config(values=config, cls=DataParameters, **kwargs) return self - def with_train(self, config: TrainingHyperparams | ParamDict | None = None, **kwargs) -> Self: + def with_train(self, config: TrainingHyperparams | RawConfig | None = None, **kwargs: object) -> Self: """Configure training hyperparameters. Args: - config: Training configuration object or dict. + config: Training configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``learning_rate``). Returns: @@ -169,11 +179,11 @@ def with_train(self, config: TrainingHyperparams | ParamDict | None = None, **kw ) return self - def with_generate(self, config: GenerateParameters | ParamDict | None = None, **kwargs) -> Self: + def with_generate(self, config: GenerateParameters | RawConfig | None = None, **kwargs: object) -> Self: """Configure generation settings. Args: - config: Generation configuration object or dict. + config: Generation configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``num_records``). Returns: @@ -184,11 +194,11 @@ def with_generate(self, config: GenerateParameters | ParamDict | None = None, ** ) return self - def with_time_series(self, config: TimeSeriesParameters | ParamDict | None = None, **kwargs) -> Self: + def with_time_series(self, config: TimeSeriesParameters | RawConfig | None = None, **kwargs: object) -> Self: """Configure time-series synthesis settings. Args: - config: Time-series configuration object or dict. + config: Time-series configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``time_column``). Returns: @@ -198,12 +208,12 @@ def with_time_series(self, config: TimeSeriesParameters | ParamDict | None = Non return self def with_differential_privacy( - self, config: DifferentialPrivacyHyperparams | ParamDict | None = None, **kwargs + self, config: DifferentialPrivacyHyperparams | RawConfig | None = None, **kwargs: object ) -> Self: """Configure differential privacy settings. Args: - config: DP configuration object or dict. + config: DP configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``epsilon``). Returns: @@ -213,7 +223,7 @@ def with_differential_privacy( return self def with_replace_pii( - self, config: PiiReplacerConfig | ParamDict | None = None, *, enable: bool = True, **kwargs + self, config: PiiReplacerConfig | RawConfig | None = None, *, enable: bool = True, **kwargs: object ) -> Self: """Configure PII replacement settings. @@ -233,7 +243,7 @@ def with_replace_pii( in ``from_params``. Args: - config: PII replacement configuration object or dict. + config: PII replacement configuration object or raw mapping. enable: When ``False``, disables PII replacement entirely and clears any previously set config. **kwargs: Field-level overrides (e.g. ``classify``). @@ -243,7 +253,7 @@ def with_replace_pii( Raises: ValueError: If ``config`` is not a ``PiiReplacerConfig``, - dict, or ``None``. + raw mapping, or ``None``. Example:: @@ -253,25 +263,26 @@ def with_replace_pii( self._replace_pii_config = None return self - cfg = None match config: - case PiiReplacerConfig() as m: - cfg = m.model_copy(update=kwargs, deep=True) - case dict() as d: - cfg = PiiReplacerConfig.model_validate(d).model_copy(update=kwargs, deep=True) + case PiiReplacerConfig() | Mapping() as values: + cfg = self._resolve_config(values=values, cls=PiiReplacerConfig, **kwargs) case None: - cfg = PiiReplacerConfig.get_default_config().model_copy(update=kwargs, deep=True) + cfg = self._resolve_config( + values=PiiReplacerConfig.get_default_config(), + cls=PiiReplacerConfig, + **kwargs, + ) case _: - raise ValueError(f"Config must be a PiiReplacerConfig, dict, or None, got {config!r}") + raise ValueError(f"Config must be a PiiReplacerConfig, raw mapping, or None, got {config!r}") self._replace_pii_config = cfg return self - def with_evaluate(self, config: EvaluationParameters | ParamDict | None = None, **kwargs) -> Self: + def with_evaluate(self, config: EvaluationParameters | RawConfig | None = None, **kwargs: object) -> Self: """Configure evaluation settings. Args: - config: Evaluation configuration object or dict. + config: Evaluation configuration object or raw mapping. **kwargs: Field-level overrides (e.g. ``enabled``). Returns: diff --git a/src/nemo_safe_synthesizer/utils.py b/src/nemo_safe_synthesizer/utils.py index 49413973f..b8b7b97d5 100644 --- a/src/nemo_safe_synthesizer/utils.py +++ b/src/nemo_safe_synthesizer/utils.py @@ -13,7 +13,7 @@ import json import os import time -from collections.abc import Callable, Generator, Iterable +from collections.abc import Callable, Generator, Iterable, Mapping from pathlib import Path from typing import TYPE_CHECKING, Any, Protocol @@ -260,12 +260,12 @@ def debug_fmt(df: pd.DataFrame) -> str: return df.head(5).to_json(orient="records", date_format="iso") -def merge_dicts(base: dict, new: dict) -> dict: +def merge_dicts(base: Mapping[str, Any], new: Mapping[str, Any]) -> dict[str, Any]: """Deep-merge two dicts, preferring values from ``new`` on conflict.""" - result = base.copy() + result = dict(base) for k, new_v in new.items(): base_v = result.get(k) - if isinstance(base_v, dict) and isinstance(new_v, dict): + if isinstance(base_v, Mapping) and isinstance(new_v, Mapping): result[k] = merge_dicts(base_v, new_v) else: result[k] = new_v diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index a67da2150..afb1d094c 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -5,11 +5,13 @@ from unittest.mock import MagicMock, patch import pytest -from pydantic import ValidationError +from pydantic import Field, ValidationError from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig from nemo_safe_synthesizer.config.training import QuantizationScheme +from nemo_safe_synthesizer.configurator.parameters import Parameters +from nemo_safe_synthesizer.errors import ParameterError def test_safe_synthesizer_parameters(monkeypatch): @@ -133,6 +135,66 @@ def test_from_params_none_disables_pii(): assert SafeSynthesizerParameters.from_params(replace_pii=None).replace_pii is None +def test_from_params_accepts_explicit_dotted_path(): + config = SafeSynthesizerParameters.from_params(**{"generation.validation.group_by_fix_unordered_records": True}) + + assert config.generation.validation.group_by_fix_unordered_records is True + + +def test_from_params_rejects_unknown_flat_parameter(): + with pytest.raises(ParameterError, match="Unknown parameter name 'not_a_parameter'"): + SafeSynthesizerParameters.from_params(not_a_parameter=True) + + +def test_from_config_patch_validates_sparse_config(): + config = SafeSynthesizerParameters.from_config_patch({"replace_pii": None}) + + assert config.replace_pii is None + + +def test_with_config_patch_merges_sparse_patch_and_keeps_defaults_implicit(): + config = SafeSynthesizerParameters.model_validate({"generation": {"num_records": 77}}) + + merged = config.with_config_patch({"generation": {"temperature": 0.7}, "training": {"batch_size": 4}}) + + assert merged.generation.num_records == 77 + assert merged.generation.temperature == 0.7 + assert merged.generation.use_structured_generation is False + assert merged.training.batch_size == 4 + assert merged.model_dump(exclude_unset=True) == { + "generation": {"num_records": 77, "temperature": 0.7}, + "training": {"batch_size": 4}, + } + + +class _LeftParameters(Parameters): + value: int = 1 + + +class _RightParameters(Parameters): + value: int = 2 + + +class _DuplicateLeafParameters(Parameters): + left: _LeftParameters = Field(default_factory=_LeftParameters) + right: _RightParameters = Field(default_factory=_RightParameters) + + +def test_parameters_get_supports_explicit_dotted_paths(): + params = _DuplicateLeafParameters() + + assert params.get("left.value") == 1 + assert params.get("right.value") == 2 + assert params.get("missing.value", "fallback") == "fallback" + + +def test_parameters_get_rejects_ambiguous_bare_leaf_names(): + params = _DuplicateLeafParameters() + + with pytest.raises(ParameterError, match="left.value.*right.value"): + params.get("value") + + def _resolve(obj: object, path: str) -> object: """Resolve a dotted attribute ``path`` (e.g. ``generation.validation.foo``).""" for part in path.split("."): diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py new file mode 100644 index 000000000..acc114666 --- /dev/null +++ b/tests/sdk/test_config_builder.py @@ -0,0 +1,43 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from typing import Any, cast + +import pytest +from pydantic import ValidationError + +from nemo_safe_synthesizer.config import GenerateParameters +from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig +from nemo_safe_synthesizer.sdk.config_builder import ConfigBuilder + + +def test_with_generate_validates_raw_config_with_kwargs(): + with pytest.raises(ValidationError, match="patience"): + ConfigBuilder().with_generate(config={"num_records": 10}, patience=0) + + +def test_with_generate_validates_typed_config_with_kwargs(): + with pytest.raises(ValidationError, match="patience"): + ConfigBuilder().with_generate(config=GenerateParameters(num_records=10), patience=0) + + +def test_with_generate_rejects_wrong_typed_config_object(): + wrong_config = cast(Any, PiiReplacerConfig.get_default_config()) + + with pytest.raises(TypeError, match="Expected GenerateParameters"): + ConfigBuilder().with_generate(config=wrong_config) + + +def test_with_replace_pii_validates_default_config_with_kwargs(): + with pytest.raises(ValidationError, match="Invalid locale"): + ConfigBuilder().with_replace_pii(globals={"locales": ["not-a-locale"]}) + + +def test_with_replace_pii_resolves_raw_config_with_kwargs(): + builder = ConfigBuilder().with_replace_pii( + config=PiiReplacerConfig.get_default_config().model_dump(), + globals={"classify": {"enable_classify": False}}, + ) + + assert builder._replace_pii_config is not None + assert builder._replace_pii_config.globals.classify.enable_classify is False From b02a025507be8816793fd7d219326fef28afe545 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 24 Jun 2026 18:21:04 +0000 Subject: [PATCH 02/19] fix: avoid deprecated-field warnings in parameter lookup Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/configurator/parameters.py | 8 ++++++-- tests/config/test_parameters.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index e559c0b1f..34ab3edec 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -114,7 +114,9 @@ def _iter_parameters(self, recursive: bool = True) -> Generator[Mapping[str, Any def _iter_field_paths(self, prefix: tuple[str, ...] = ()) -> Iterator[tuple[tuple[str, ...], Any]]: """Yield every field path and value in this parameter tree.""" for name in type(self).model_fields: - value = getattr(self, name) + value = self.__dict__.get(name, _MISSING) + if value is _MISSING: + continue path = (*prefix, name) yield path, value if isinstance(value, Parameters): @@ -126,7 +128,9 @@ def _get_field_path(self, path: tuple[str, ...]) -> object: for part in path: if not isinstance(value, Parameters) or part not in type(value).model_fields: return _MISSING - value = getattr(value, part) + value = value.__dict__.get(part, _MISSING) + if value is _MISSING: + return _MISSING return value def __iter__(self) -> Iterator[Mapping[str, Any]]: # ty: ignore[invalid-method-override] -- intentionally overrides pydantic BaseModel.__iter__ with parameter-group semantics diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index afb1d094c..3f44a8a54 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -1,6 +1,7 @@ # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +import warnings from pathlib import Path from unittest.mock import MagicMock, patch @@ -195,6 +196,16 @@ def test_parameters_get_rejects_ambiguous_bare_leaf_names(): params.get("value") +def test_parameters_get_does_not_warn_for_unrelated_deprecated_fields(): + params = SafeSynthesizerParameters() + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", DeprecationWarning) + assert params.get("batch_size") == params.training.batch_size + + assert not [warning for warning in caught if issubclass(warning.category, DeprecationWarning)] + + def _resolve(obj: object, path: str) -> object: """Resolve a dotted attribute ``path`` (e.g. ``generation.validation.foo``).""" for part in path.split("."): From d7d88d95f306db96937b9cbb04e3c4209a2140cf Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 24 Jun 2026 21:41:14 +0000 Subject: [PATCH 03/19] fix: reject ambiguous config parameter lookups Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 5 ++--- .../configurator/parameters.py | 9 ++++++-- tests/config/test_parameters.py | 22 +++++++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index dff90c3e3..8664f95c6 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -77,11 +77,10 @@ def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object target[head] = value return - next_value = target.get(head) - if next_value is None: + if head not in target: nested: dict[str, object] = {} target[head] = nested - elif isinstance(next_value, dict): + elif isinstance(next_value := target[head], dict): nested = {str(key): item for key, item in next_value.items()} target[head] = nested else: diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index 34ab3edec..f1284861b 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -167,7 +167,8 @@ def has(self, name: str) -> bool: """Check whether ``name`` exists anywhere in the parameter tree. Unlike ``get()``, this does not conflate falsy values (``0``, ``""``, - ``False``, ``None``) with absence. + ``False``, ``None``) with absence. Bare names are accepted only when + they map to at most one field in the parameter tree. Args: name: Field name to search for. @@ -177,7 +178,11 @@ def has(self, name: str) -> bool: """ if "." in name: return self._get_field_path(tuple(name.split("."))) is not _MISSING - return any(path[-1] == name for path, _ in self._iter_field_paths()) + matches = [path for path, _ in self._iter_field_paths() if path[-1] == name] + if len(matches) > 1: + candidates = ", ".join(".".join(path) for path in matches) + raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") + return bool(matches) @classmethod def from_yaml_str(cls, raw: str) -> Self: diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 3f44a8a54..29c52bafc 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -142,6 +142,13 @@ def test_from_params_accepts_explicit_dotted_path(): assert config.generation.validation.group_by_fix_unordered_records is True +def test_from_params_rejects_top_level_none_before_nested_override(): + with pytest.raises( + ParameterError, match="Cannot assign nested parameter path 'generation.num_records'.*'generation'" + ): + SafeSynthesizerParameters.from_params(generation=None, **{"generation.num_records": 10}) + + def test_from_params_rejects_unknown_flat_parameter(): with pytest.raises(ParameterError, match="Unknown parameter name 'not_a_parameter'"): SafeSynthesizerParameters.from_params(not_a_parameter=True) @@ -196,6 +203,21 @@ def test_parameters_get_rejects_ambiguous_bare_leaf_names(): params.get("value") +def test_parameters_has_supports_explicit_dotted_paths(): + params = _DuplicateLeafParameters() + + assert params.has("left.value") is True + assert params.has("right.value") is True + assert params.has("missing.value") is False + + +def test_parameters_has_rejects_ambiguous_bare_leaf_names(): + params = _DuplicateLeafParameters() + + with pytest.raises(ParameterError, match="left.value.*right.value"): + params.has("value") + + def test_parameters_get_does_not_warn_for_unrelated_deprecated_fields(): params = SafeSynthesizerParameters() From 499e84d7ffd19c275e4b0487fa031b63c2e06a2a Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 24 Jun 2026 22:04:37 +0000 Subject: [PATCH 04/19] fix: make from_params overrides order independent Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 44 ++++++++++++------- tests/config/test_parameters.py | 33 ++++++++++++++ 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 8664f95c6..bc1e8df13 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -70,6 +70,15 @@ def _overlay_set_fields(saved: Parameters, runtime: Parameters) -> Parameters: return saved.model_validate(merge_dicts(saved.model_dump(), overrides)) +def _section_patch(value: object) -> _SectionPatch | None: + """Convert a section value to a mutable patch dict when nested fields can merge into it.""" + if isinstance(value, BaseModel): + return value.model_dump(exclude_unset=True) + if isinstance(value, Mapping): + return {str(key): item for key, item in value.items()} + return None + + def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object) -> None: """Assign ``value`` into ``target`` at a dotted config path.""" head, *tail = path @@ -80,8 +89,7 @@ def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object if head not in target: nested: dict[str, object] = {} target[head] = nested - elif isinstance(next_value := target[head], dict): - nested = {str(key): item for key, item in next_value.items()} + elif (nested := _section_patch(target[head])) is not None: target[head] = nested else: raise ParameterError(f"Cannot assign nested parameter path {'.'.join(path)!r}; {head!r} is already set.") @@ -253,21 +261,27 @@ def from_params(cls, **kwargs) -> "SafeSynthesizerParameters": for path, _ in section._iter_field_paths((section_name,)): field_index.setdefault(path[-1], []).append(path) - patch: dict[str, object] = {} + path_assignments: dict[tuple[str, ...], object] = {} for name, value in kwargs.items(): if "." in name: - _assign_path(patch, tuple(name.split(".")), value) - continue - if name in top_level_fields: - patch[name] = value - continue - matches = field_index.get(name, []) - if not matches: - raise ParameterError(f"Unknown parameter name {name!r}.") - if len(matches) > 1: - candidates = ", ".join(".".join(path) for path in matches) - raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") - _assign_path(patch, matches[0], value) + path = tuple(name.split(".")) + elif name in top_level_fields: + path = (name,) + else: + matches = field_index.get(name, []) + if not matches: + raise ParameterError(f"Unknown parameter name {name!r}.") + if len(matches) > 1: + candidates = ", ".join(".".join(path) for path in matches) + raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") + path = matches[0] + if path in path_assignments: + raise ParameterError(f"Duplicate parameter path {'.'.join(path)!r}.") + path_assignments[path] = value + + patch: dict[str, object] = {} + for path, value in sorted(path_assignments.items(), key=lambda item: len(item[0])): + _assign_path(patch, path, value) return cls.model_validate(patch) diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 29c52bafc..aa14c795d 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -149,6 +149,39 @@ def test_from_params_rejects_top_level_none_before_nested_override(): SafeSynthesizerParameters.from_params(generation=None, **{"generation.num_records": 10}) +@pytest.mark.parametrize( + "kwargs", + [ + pytest.param({"generation": {"temperature": 0.7}, "generation.num_records": 10}, id="section-then-dotted"), + pytest.param({"generation.num_records": 10, "generation": {"temperature": 0.7}}, id="dotted-then-section"), + pytest.param({"generation": {"temperature": 0.7}, "num_records": 10}, id="section-then-bare-leaf"), + pytest.param({"num_records": 10, "generation": {"temperature": 0.7}}, id="bare-leaf-then-section"), + ], +) +def test_from_params_merges_section_and_leaf_overrides_order_independently(kwargs: dict[str, object]): + config = SafeSynthesizerParameters.from_params(**kwargs) + + assert config.generation.num_records == 10 + assert config.generation.temperature == 0.7 + + +@pytest.mark.parametrize( + "kwargs", + [ + pytest.param({"privacy": None, "privacy.dp_enabled": True}, id="none-then-dotted"), + pytest.param({"privacy.dp_enabled": True, "privacy": None}, id="dotted-then-none"), + ], +) +def test_from_params_rejects_section_none_with_nested_override(kwargs: dict[str, object]): + with pytest.raises(ParameterError, match="Cannot assign nested parameter path 'privacy.dp_enabled'.*'privacy'"): + SafeSynthesizerParameters.from_params(**kwargs) + + +def test_from_params_rejects_duplicate_specific_parameter_paths(): + with pytest.raises(ParameterError, match="Duplicate parameter path 'generation.num_records'"): + SafeSynthesizerParameters.from_params(num_records=10, **{"generation.num_records": 20}) + + def test_from_params_rejects_unknown_flat_parameter(): with pytest.raises(ParameterError, match="Unknown parameter name 'not_a_parameter'"): SafeSynthesizerParameters.from_params(not_a_parameter=True) From 56bd01a93088a507ffdcf574ef46c111ffe1365c Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 24 Jun 2026 22:11:15 +0000 Subject: [PATCH 05/19] chore: type from_params kwargs Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/config/parameters.py | 2 +- src/nemo_safe_synthesizer/configurator/parameters.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index bc1e8df13..79e9b9b4c 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -224,7 +224,7 @@ def check_timeseries_group_column(self) -> Self: @classmethod @override - def from_params(cls, **kwargs) -> "SafeSynthesizerParameters": + def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": """Convert singular, flat parameters to nested structure. Takes a flat dictionary of parameters, where keys correspond to diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index f1284861b..f5e1bd357 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -251,7 +251,7 @@ def to_yaml(self, path: PathT, exclude_unset: bool = True) -> None: yaml.safe_dump(j, f) @classmethod - def from_params(cls, **kwargs) -> Self: + def from_params(cls, **kwargs: object) -> Self: """Construct a ``Parameters`` instance from keyword arguments. Args: From 262bfd366e9e88369a77203d2402b1ef13ebb533 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Thu, 25 Jun 2026 22:29:16 +0000 Subject: [PATCH 06/19] fix(sdk): preserve sparse typed config overrides Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/sdk/config_builder.py | 2 +- tests/sdk/test_config_builder.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index d6f124f29..1e088f057 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -129,7 +129,7 @@ def _resolve_config(self, values: object, cls: type[ParamT], **kwargs: object) - case BaseModel() as model: if not isinstance(model, cls): raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") - raw_values = model.model_dump() + raw_values = model.model_dump(exclude_unset=True) raw_values.update(kwargs) return cls.model_validate(raw_values) case Mapping() as mapping: diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py index acc114666..d4319bdb0 100644 --- a/tests/sdk/test_config_builder.py +++ b/tests/sdk/test_config_builder.py @@ -21,6 +21,20 @@ def test_with_generate_validates_typed_config_with_kwargs(): ConfigBuilder().with_generate(config=GenerateParameters(num_records=10), patience=0) +def test_with_generate_preserves_sparse_typed_config_fields(): + builder = ConfigBuilder().with_generate(config=GenerateParameters(num_records=10)) + + assert builder._generation_config is not None + assert builder._generation_config.__pydantic_fields_set__ == {"num_records"} + + +def test_with_generate_marks_typed_config_kwargs_as_explicit_fields(): + builder = ConfigBuilder().with_generate(config=GenerateParameters(num_records=10), patience=7) + + assert builder._generation_config is not None + assert builder._generation_config.__pydantic_fields_set__ == {"num_records", "patience"} + + def test_with_generate_rejects_wrong_typed_config_object(): wrong_config = cast(Any, PiiReplacerConfig.get_default_config()) From 93a4276dd81110f50243fcb51dbb0beac03b314b Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Fri, 26 Jun 2026 20:42:18 +0000 Subject: [PATCH 07/19] fix(config): preserve structured generation aliases Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/config/parameters.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 79e9b9b4c..4bfb472a7 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -96,6 +96,14 @@ def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object _assign_path(nested, tuple(tail), value) +_LEGACY_FLAT_PATHS: dict[str, tuple[str, ...]] = { + "use_structured_generation": ("generation", "structured_generation", "enabled"), + "structured_generation_backend": ("generation", "structured_generation", "backend"), + "structured_generation_schema_method": ("generation", "structured_generation", "schema_method"), + "structured_generation_use_single_sequence": ("generation", "structured_generation", "use_single_sequence"), +} + + class SafeSynthesizerParameters(Parameters): """Main configuration class for the Safe Synthesizer pipeline. @@ -267,6 +275,8 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": path = tuple(name.split(".")) elif name in top_level_fields: path = (name,) + elif name in _LEGACY_FLAT_PATHS: + path = _LEGACY_FLAT_PATHS[name] else: matches = field_index.get(name, []) if not matches: From b02f4023c2362a8c2fb04f3ee763407686870699 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Mon, 29 Jun 2026 23:44:24 +0000 Subject: [PATCH 08/19] fix(config): address review findings Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/config/parameters.py | 3 +++ src/nemo_safe_synthesizer/sdk/config_builder.py | 7 +++---- tests/config/test_parameters.py | 15 ++++++++++----- tests/sdk/test_config_builder.py | 16 ++++++++++++++++ 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 4bfb472a7..aac8b0bea 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -264,6 +264,7 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": "preflight": PreflightParameters(), } top_level_fields = set(cls.model_fields) + default_params = cls() field_index: dict[str, list[tuple[str, ...]]] = {} for section_name, section in section_defaults.items(): for path, _ in section._iter_field_paths((section_name,)): @@ -273,6 +274,8 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": for name, value in kwargs.items(): if "." in name: path = tuple(name.split(".")) + if not default_params.has(name): + raise ParameterError(f"Unknown parameter path {name!r}.") elif name in top_level_fields: path = (name,) elif name in _LEGACY_FLAT_PATHS: diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index 1e088f057..6b8d2bf32 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -23,6 +23,7 @@ ) from ..observability import get_logger from ..telemetry import _telemetry_enabled +from ..utils import merge_dicts logger = get_logger(__name__) @@ -130,12 +131,10 @@ def _resolve_config(self, values: object, cls: type[ParamT], **kwargs: object) - if not isinstance(model, cls): raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") raw_values = model.model_dump(exclude_unset=True) - raw_values.update(kwargs) - return cls.model_validate(raw_values) + return cls.model_validate(merge_dicts(raw_values, kwargs)) case Mapping() as mapping: raw_values = dict(mapping) - raw_values.update(kwargs) - return cls.model_validate(raw_values) + return cls.model_validate(merge_dicts(raw_values, kwargs)) case _: raise TypeError(f"Unsupported config type: {type(values)}") diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index aa14c795d..948a6bbe5 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -142,9 +142,14 @@ def test_from_params_accepts_explicit_dotted_path(): assert config.generation.validation.group_by_fix_unordered_records is True +def test_from_params_rejects_unknown_explicit_dotted_path(): + with pytest.raises(ParameterError, match=r"Unknown parameter path 'generation\.not_a_field'"): + SafeSynthesizerParameters.from_params(**{"generation.not_a_field": True}) + + def test_from_params_rejects_top_level_none_before_nested_override(): with pytest.raises( - ParameterError, match="Cannot assign nested parameter path 'generation.num_records'.*'generation'" + ParameterError, match=r"Cannot assign nested parameter path 'generation\.num_records'.*'generation'" ): SafeSynthesizerParameters.from_params(generation=None, **{"generation.num_records": 10}) @@ -173,12 +178,12 @@ def test_from_params_merges_section_and_leaf_overrides_order_independently(kwarg ], ) def test_from_params_rejects_section_none_with_nested_override(kwargs: dict[str, object]): - with pytest.raises(ParameterError, match="Cannot assign nested parameter path 'privacy.dp_enabled'.*'privacy'"): + with pytest.raises(ParameterError, match=r"Cannot assign nested parameter path 'privacy\.dp_enabled'.*'privacy'"): SafeSynthesizerParameters.from_params(**kwargs) def test_from_params_rejects_duplicate_specific_parameter_paths(): - with pytest.raises(ParameterError, match="Duplicate parameter path 'generation.num_records'"): + with pytest.raises(ParameterError, match=r"Duplicate parameter path 'generation\.num_records'"): SafeSynthesizerParameters.from_params(num_records=10, **{"generation.num_records": 20}) @@ -232,7 +237,7 @@ def test_parameters_get_supports_explicit_dotted_paths(): def test_parameters_get_rejects_ambiguous_bare_leaf_names(): params = _DuplicateLeafParameters() - with pytest.raises(ParameterError, match="left.value.*right.value"): + with pytest.raises(ParameterError, match=r"left\.value.*right\.value"): params.get("value") @@ -247,7 +252,7 @@ def test_parameters_has_supports_explicit_dotted_paths(): def test_parameters_has_rejects_ambiguous_bare_leaf_names(): params = _DuplicateLeafParameters() - with pytest.raises(ParameterError, match="left.value.*right.value"): + with pytest.raises(ParameterError, match=r"left\.value.*right\.value"): params.has("value") diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py index d4319bdb0..506772faa 100644 --- a/tests/sdk/test_config_builder.py +++ b/tests/sdk/test_config_builder.py @@ -55,3 +55,19 @@ def test_with_replace_pii_resolves_raw_config_with_kwargs(): assert builder._replace_pii_config is not None assert builder._replace_pii_config.globals.classify.enable_classify is False + + +@pytest.mark.parametrize("as_mapping", [False, True]) +def test_with_replace_pii_deep_merges_nested_kwargs(as_mapping: bool): + config_model = PiiReplacerConfig.get_default_config() + config_model.globals.locales = ["en_US"] + config = config_model.model_dump() if as_mapping else config_model + + builder = ConfigBuilder().with_replace_pii( + config=config, + globals={"classify": {"enable_classify": False}}, + ) + + assert builder._replace_pii_config is not None + assert builder._replace_pii_config.globals.locales == ["en_US"] + assert builder._replace_pii_config.globals.classify.enable_classify is False From 22f4786039c09d6f35c011a8e0edfab5144192ec Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 00:14:10 +0000 Subject: [PATCH 09/19] refactor(config): centralize parameter path resolution Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 65 +++---- .../configurator/parameter_paths.py | 174 ++++++++++++++++++ .../configurator/pydantic_click_options.py | 21 +-- tests/config/test_generate.py | 8 + tests/config/test_parameters.py | 47 ++++- .../test_pydantic_click_options.py | 6 + 6 files changed, 269 insertions(+), 52 deletions(-) create mode 100644 src/nemo_safe_synthesizer/configurator/parameter_paths.py diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index aac8b0bea..a9c5d5829 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -10,6 +10,13 @@ from pydantic import BaseModel, Field, model_validator from typing_extensions import override +from ..configurator.parameter_paths import ( + AmbiguousParameterName, + ParameterPath, + ParameterSchema, + ResolvedParameterName, + UnknownParameterName, +) from ..configurator.parameters import Parameters from ..errors import ParameterError from ..observability import get_logger @@ -104,6 +111,22 @@ def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object } +def _resolve_parameter_name(schema: ParameterSchema, name: str) -> ParameterPath: + """Resolve one ``from_params`` name while retaining legacy alias policy.""" + if name not in schema.model_type.model_fields and name in _LEGACY_FLAT_PATHS: + return ParameterPath(_LEGACY_FLAT_PATHS[name]) + + match schema.resolve(name): + case ResolvedParameterName() as resolved: + return resolved.path + case UnknownParameterName() as unknown: + kind = "path" if "." in name else "name" + raise ParameterError(f"Unknown parameter {kind} {unknown.name!r}.") + case AmbiguousParameterName() as ambiguous: + choices = ", ".join(str(path) for path in ambiguous.candidates) + raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") + + class SafeSynthesizerParameters(Parameters): """Main configuration class for the Safe Synthesizer pipeline. @@ -254,47 +277,17 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": >>> from nemo_safe_synthesizer.config import SafeSynthesizerParameters >>> SafeSynthesizerParameters.from_params(structured_generation={"enabled": True}) """ - section_defaults: dict[str, Parameters] = { - "training": TrainingHyperparams(), - "generation": GenerateParameters(), - "evaluation": EvaluationParameters(), - "privacy": DifferentialPrivacyHyperparams(), - "data": DataParameters(), - "time_series": TimeSeriesParameters(), - "preflight": PreflightParameters(), - } - top_level_fields = set(cls.model_fields) - default_params = cls() - field_index: dict[str, list[tuple[str, ...]]] = {} - for section_name, section in section_defaults.items(): - for path, _ in section._iter_field_paths((section_name,)): - field_index.setdefault(path[-1], []).append(path) - - path_assignments: dict[tuple[str, ...], object] = {} + schema = ParameterSchema.from_model(cls) + path_assignments: dict[ParameterPath, object] = {} for name, value in kwargs.items(): - if "." in name: - path = tuple(name.split(".")) - if not default_params.has(name): - raise ParameterError(f"Unknown parameter path {name!r}.") - elif name in top_level_fields: - path = (name,) - elif name in _LEGACY_FLAT_PATHS: - path = _LEGACY_FLAT_PATHS[name] - else: - matches = field_index.get(name, []) - if not matches: - raise ParameterError(f"Unknown parameter name {name!r}.") - if len(matches) > 1: - candidates = ", ".join(".".join(path) for path in matches) - raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") - path = matches[0] + path = _resolve_parameter_name(schema, name) if path in path_assignments: - raise ParameterError(f"Duplicate parameter path {'.'.join(path)!r}.") + raise ParameterError(f"Duplicate parameter path {str(path)!r}.") path_assignments[path] = value patch: dict[str, object] = {} - for path, value in sorted(path_assignments.items(), key=lambda item: len(item[0])): - _assign_path(patch, path, value) + for path, value in sorted(path_assignments.items(), key=lambda item: len(item[0].parts)): + _assign_path(patch, path.parts, value) return cls.model_validate(patch) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py new file mode 100644 index 000000000..4586c7c3e --- /dev/null +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -0,0 +1,174 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Private schema-aware parameter path primitives.""" + +from __future__ import annotations + +import types +from dataclasses import dataclass +from enum import Enum, auto +from typing import TYPE_CHECKING, Annotated, Self, Union, cast, get_args, get_origin + +if TYPE_CHECKING: + from .parameters import Parameters + + +@dataclass(frozen=True, slots=True) +class ParameterPath: + """Canonical path to a parameter field.""" + + parts: tuple[str, ...] + + def __post_init__(self) -> None: + if not self.parts or any(not part for part in self.parts): + raise ValueError("A parameter path cannot contain empty segments.") + + def __str__(self) -> str: + return ".".join(self.parts) + + +class ParameterFieldKind(Enum): + """Schema classification for a parameter field.""" + + BRANCH = auto() + LEAF = auto() + + +def classify_parameter_annotation(annotation: object) -> ParameterFieldKind: + """Classify a Pydantic field annotation as a branch or leaf.""" + if _nested_parameters_type(annotation) is not None: + return ParameterFieldKind.BRANCH + return ParameterFieldKind.LEAF + + +def _unwrap_annotated(annotation: object) -> object: + while get_origin(annotation) is Annotated: + annotation = get_args(annotation)[0] + return annotation + + +def _nested_parameters_type(annotation: object) -> type[Parameters] | None: + from .parameters import Parameters + + annotation = _unwrap_annotated(annotation) + origin = get_origin(annotation) + if origin in (types.UnionType, Union): + members = tuple(_unwrap_annotated(member) for member in get_args(annotation) if member is not type(None)) + if len(members) != 1: + return None + annotation = members[0] + if isinstance(annotation, type) and issubclass(annotation, Parameters): + return annotation + return None + + +@dataclass(frozen=True, slots=True) +class ParameterField: + """One indexed field in a parameter schema.""" + + path: ParameterPath + kind: ParameterFieldKind + + +@dataclass(frozen=True, slots=True) +class ResolvedParameterName: + """A parameter name resolved to one canonical path.""" + + path: ParameterPath + + +@dataclass(frozen=True, slots=True) +class UnknownParameterName: + """A parameter name not present in the schema.""" + + name: str + + +@dataclass(frozen=True, slots=True) +class AmbiguousParameterName: + """A bare parameter name with multiple canonical candidates.""" + + name: str + candidates: tuple[ParameterPath, ...] + + +ParameterNameResolution = ResolvedParameterName | UnknownParameterName | AmbiguousParameterName + + +@dataclass(frozen=True, slots=True) +class ParameterSchema: + """Indexed field paths for one ``Parameters`` model type.""" + + model_type: type[Parameters] + fields: tuple[ParameterField, ...] + + @classmethod + def from_model(cls, model_type: type[Parameters]) -> Self: + """Build a schema from Pydantic field annotations.""" + from .parameters import Parameters + + if not issubclass(model_type, Parameters): + raise TypeError(f"Expected a Parameters model type, received {model_type!r}.") + fields = tuple(_iter_parameter_fields(model_type)) + return cls(model_type=model_type, fields=fields) + + def resolve(self, name: str) -> ParameterNameResolution: + """Resolve a canonical dotted or bare parameter name.""" + if "." in name: + try: + requested = split_parameter_path(name) + except ValueError: + return UnknownParameterName(name) + if any(field.path == requested for field in self.fields): + return ResolvedParameterName(requested) + return UnknownParameterName(name) + + top_level = next((field.path for field in self.fields if field.path.parts == (name,)), None) + if top_level is not None: + return ResolvedParameterName(top_level) + candidates = tuple(field.path for field in self.fields if field.path.parts[-1] == name) + if not candidates: + return UnknownParameterName(name) + if len(candidates) > 1: + return AmbiguousParameterName(name, candidates) + return ResolvedParameterName(candidates[0]) + + +def _iter_parameter_fields( + model_type: type[Parameters], prefix: tuple[str, ...] = () +) -> tuple[ParameterField, ...]: + fields: list[ParameterField] = [] + for name, field_info in model_type.model_fields.items(): + path = ParameterPath((*prefix, name)) + kind = classify_parameter_annotation(field_info.annotation) + fields.append(ParameterField(path, kind)) + if kind is ParameterFieldKind.BRANCH: + nested_type = _nested_parameters_type(field_info.annotation) + if nested_type is not None: + fields.extend(_iter_parameter_fields(nested_type, path.parts)) + return tuple(fields) + + +def split_parameter_path(name: str, separator: str = ".") -> ParameterPath: + """Split a parameter name into a canonical path.""" + if not separator: + raise ValueError("A parameter path separator cannot be empty.") + parts = tuple(name.split(separator)) + if any(not part for part in parts): + raise ValueError(f"Invalid parameter path {name!r}: empty segment.") + return ParameterPath(parts) + + +def insert_parameter_value(target: dict[str, object], path: ParameterPath, value: object) -> None: + """Insert a value at an already resolved path.""" + current = target + for part in path.parts[:-1]: + value_at_part = current.get(part) + if isinstance(value_at_part, dict): + current = cast(dict[str, object], value_at_part) + continue + nested: dict[str, object] = {} + current[part] = nested + current = nested + current[path.parts[-1]] = value diff --git a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py index 073f29bfe..5143aa5f3 100644 --- a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py +++ b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py @@ -29,6 +29,7 @@ from typing_extensions import TypeIs from ..config.types import AUTO_STR +from .parameter_paths import insert_parameter_value, split_parameter_path __all__ = ["pydantic_options", "parse_overrides", "AutoParamType"] @@ -70,21 +71,11 @@ def parse_overrides(values: dict[str, Any] | None = None, field_sep: str = "__") continue if v is None: continue - match k.split(field_sep): - case [key]: - overrides[key] = v - case [first, *rest, last] if all(rest) and last: - target = overrides - if not isinstance(target.get(first), dict): - target[first] = {} - target = target[first] - for part in rest: - if not isinstance(target.get(part), dict): - target[part] = {} - target = target[part] - target[last] = v - case _: - raise ValueError(f"Invalid override key: {k!r}") + try: + path = split_parameter_path(k, field_sep) + except ValueError as error: + raise ValueError(f"Invalid override key: {k!r}") from error + insert_parameter_value(overrides, path, v) return overrides diff --git a/tests/config/test_generate.py b/tests/config/test_generate.py index 8f8fc5bc2..e0418a371 100644 --- a/tests/config/test_generate.py +++ b/tests/config/test_generate.py @@ -12,6 +12,7 @@ resolve_structured_generation_schema_method, ) from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters +from nemo_safe_synthesizer.errors import ParameterError @pytest.mark.unit @@ -180,6 +181,13 @@ def test_from_params_legacy_enabled_overrides_structured_generation_kwarg(self) assert params.generation.structured_generation.enabled is True assert params.generation.structured_generation.schema_method == "json_schema" # preserved + def test_from_params_legacy_alias_and_dotted_name_are_duplicate_paths(self) -> None: + with pytest.raises(ParameterError, match=r"generation\.structured_generation\.backend"): + SafeSynthesizerParameters.from_params( + structured_generation_backend="xgrammar", + **{"generation.structured_generation.backend": "outlines"}, + ) + def test_nested_dict_with_no_legacy_keys_uses_dict_values(self) -> None: """When no legacy flat keys are present, nested dict values are used as-is.""" params = GenerateParameters.model_validate( diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 948a6bbe5..1b3b8831c 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -9,8 +9,16 @@ from pydantic import Field, ValidationError from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters -from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig +from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig, StepDefinition from nemo_safe_synthesizer.config.training import QuantizationScheme +from nemo_safe_synthesizer.configurator.parameter_paths import ( + AmbiguousParameterName, + ParameterFieldKind, + ParameterSchema, + ResolvedParameterName, + UnknownParameterName, + classify_parameter_annotation, +) from nemo_safe_synthesizer.configurator.parameters import Parameters from nemo_safe_synthesizer.errors import ParameterError @@ -192,6 +200,43 @@ def test_from_params_rejects_unknown_flat_parameter(): SafeSynthesizerParameters.from_params(not_a_parameter=True) +def test_parameter_schema_indexes_optional_branches_without_an_instance(): + with patch.object(SafeSynthesizerParameters, "__init__", side_effect=AssertionError("model instantiated")): + schema = ParameterSchema.from_model(SafeSynthesizerParameters) + fields = {str(field.path): field.kind for field in schema.fields} + + assert "privacy" in fields + assert fields["privacy"] is ParameterFieldKind.BRANCH + assert fields["privacy.dp_enabled"] is ParameterFieldKind.LEAF + assert isinstance(schema.resolve("privacy.dp_enabled"), ResolvedParameterName) + assert isinstance(schema.resolve("privacy.not_a_field"), UnknownParameterName) + + +def test_parameter_schema_reports_ambiguous_bare_names_with_candidates(): + result = ParameterSchema.from_model(SafeSynthesizerParameters).resolve("enabled") + + assert isinstance(result, AmbiguousParameterName) + assert {str(path) for path in result.candidates} >= { + "evaluation.enabled", + "generation.structured_generation.enabled", + } + + +def test_mapping_valued_step_vars_annotation_is_a_leaf(): + annotation = StepDefinition.model_fields["vars"].annotation + + assert classify_parameter_annotation(annotation) is ParameterFieldKind.LEAF + + +def test_parameters_queries_remain_instance_based_for_disabled_optional_branch(): + params = SafeSynthesizerParameters(privacy=None) + schema_result = ParameterSchema.from_model(SafeSynthesizerParameters).resolve("privacy.dp_enabled") + + assert params.get("privacy.dp_enabled", "missing") == "missing" + assert params.has("privacy.dp_enabled") is False + assert isinstance(schema_result, ResolvedParameterName) + + def test_from_config_patch_validates_sparse_config(): config = SafeSynthesizerParameters.from_config_patch({"replace_pii": None}) diff --git a/tests/configurator/test_pydantic_click_options.py b/tests/configurator/test_pydantic_click_options.py index 78ebe98e0..8b3999605 100644 --- a/tests/configurator/test_pydantic_click_options.py +++ b/tests/configurator/test_pydantic_click_options.py @@ -120,6 +120,12 @@ def test_parse_overrides_empty_segment_raises(): parse_overrides({"a____b": "x"}) +@pytest.mark.parametrize("key", [".a", "a.", "a..b"]) +def test_parse_overrides_custom_separator_rejects_empty_segments(key: str): + with pytest.raises(ValueError, match=repr(key)): + parse_overrides({key: "x"}, field_sep=".") + + def test_parse_overrides_no_flag_then_nested_override(): """Nested override takes precedence over --no_ flag for same field.""" result = parse_overrides({"no_privacy": True, "privacy__epsilon": 1.0}) From 76b1f2949d9e675e2c6fccd1dc4ca9fdc87b5c51 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 00:53:50 +0000 Subject: [PATCH 10/19] refactor(config): compile schema-aware config patches Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 54 ++-- src/nemo_safe_synthesizer/config/patch.py | 263 ++++++++++++++++++ .../configurator/parameter_paths.py | 4 +- tests/cli/test_utils.py | 37 ++- tests/config/test_parameters.py | 36 ++- tests/config/test_patch.py | 233 ++++++++++++++++ 6 files changed, 578 insertions(+), 49 deletions(-) create mode 100644 src/nemo_safe_synthesizer/config/patch.py create mode 100644 tests/config/test_patch.py diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index a9c5d5829..66b96458b 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -26,6 +26,7 @@ from .differential_privacy import DifferentialPrivacyHyperparams from .evaluate import EvaluationParameters from .generate import GenerateParameters +from .patch import CompiledConfigPatch, PatchAssignment from .preflight import PreflightParameters from .replace_pii import PiiReplacerConfig from .time_series import TimeSeriesParameters @@ -77,32 +78,6 @@ def _overlay_set_fields(saved: Parameters, runtime: Parameters) -> Parameters: return saved.model_validate(merge_dicts(saved.model_dump(), overrides)) -def _section_patch(value: object) -> _SectionPatch | None: - """Convert a section value to a mutable patch dict when nested fields can merge into it.""" - if isinstance(value, BaseModel): - return value.model_dump(exclude_unset=True) - if isinstance(value, Mapping): - return {str(key): item for key, item in value.items()} - return None - - -def _assign_path(target: dict[str, object], path: tuple[str, ...], value: object) -> None: - """Assign ``value`` into ``target`` at a dotted config path.""" - head, *tail = path - if not tail: - target[head] = value - return - - if head not in target: - nested: dict[str, object] = {} - target[head] = nested - elif (nested := _section_patch(target[head])) is not None: - target[head] = nested - else: - raise ParameterError(f"Cannot assign nested parameter path {'.'.join(path)!r}; {head!r} is already set.") - _assign_path(nested, tuple(tail), value) - - _LEGACY_FLAT_PATHS: dict[str, tuple[str, ...]] = { "use_structured_generation": ("generation", "structured_generation", "enabled"), "structured_generation_backend": ("generation", "structured_generation", "backend"), @@ -278,23 +253,21 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": >>> SafeSynthesizerParameters.from_params(structured_generation={"enabled": True}) """ schema = ParameterSchema.from_model(cls) - path_assignments: dict[ParameterPath, object] = {} + assignments: list[PatchAssignment] = [] + resolved_paths: set[ParameterPath] = set() for name, value in kwargs.items(): path = _resolve_parameter_name(schema, name) - if path in path_assignments: + if path in resolved_paths: raise ParameterError(f"Duplicate parameter path {str(path)!r}.") - path_assignments[path] = value - - patch: dict[str, object] = {} - for path, value in sorted(path_assignments.items(), key=lambda item: len(item[0].parts)): - _assign_path(patch, path.parts, value) + resolved_paths.add(path) + assignments.append(PatchAssignment(path, value, f"parameter {name!r}", 0)) - return cls.model_validate(patch) + return CompiledConfigPatch.from_paths(cls, assignments).apply() @classmethod def from_config_patch(cls, patch: ConfigPatch) -> Self: """Validate a sparse top-level config patch as a full configuration.""" - return cls.model_validate(patch) + return CompiledConfigPatch.from_mapping(cls, patch, origin="config patch", precedence=0).apply() def with_config_patch(self, patch: ConfigPatch) -> Self: """Apply a sparse top-level config patch and revalidate the result. @@ -303,8 +276,15 @@ def with_config_patch(self, patch: ConfigPatch) -> Self: applying ``patch``. This preserves file/CLI precedence while keeping default values implicit for future ``exclude_unset`` dumps. """ - params = merge_dicts(self.model_dump(exclude_unset=True), patch) - return type(self).model_validate(params) + model_type = type(self) + base = CompiledConfigPatch.from_mapping( + model_type, + self.model_dump(exclude_unset=True), + origin="base config", + precedence=0, + ) + override = CompiledConfigPatch.from_mapping(model_type, patch, origin="config patch", precedence=1) + return base.combine(override).apply() def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSynthesizerParameters": """Apply resume-time generation/evaluation/telemetry overrides onto a copy of self. diff --git a/src/nemo_safe_synthesizer/config/patch.py b/src/nemo_safe_synthesizer/config/patch.py new file mode 100644 index 000000000..11e96d493 --- /dev/null +++ b/src/nemo_safe_synthesizer/config/patch.py @@ -0,0 +1,263 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Private schema-aware configuration patch primitives.""" + +from __future__ import annotations + +import types +from collections.abc import Iterable, Mapping +from copy import deepcopy +from dataclasses import dataclass +from typing import Annotated, Generic, TypeVar, Union, cast, get_args, get_origin + +from pydantic import BaseModel + +from ..configurator.parameter_paths import ParameterPath +from ..errors import ParameterError + +ModelT = TypeVar("ModelT", bound=BaseModel) + + +@dataclass(frozen=True, slots=True) +class PatchAssignment: + """One canonical configuration assignment with source precedence.""" + + path: ParameterPath + value: object + origin: str + precedence: int + + +@dataclass(frozen=True, slots=True) +class CompiledConfigPatch(Generic[ModelT]): + """A configuration patch compiled for one exact Pydantic model type.""" + + target_model: type[ModelT] + assignments: tuple[PatchAssignment, ...] + + @staticmethod + def from_paths(target_model: type[ModelT], assignments: Iterable[PatchAssignment]) -> CompiledConfigPatch[ModelT]: + _require_model_type(target_model) + copied = tuple( + PatchAssignment(item.path, deepcopy(item.value), item.origin, item.precedence) for item in assignments + ) + for assignment in copied: + _field_model_at_path(target_model, assignment.path) + _validate_conflicts(target_model, copied) + return CompiledConfigPatch(target_model=target_model, assignments=copied) + + @staticmethod + def from_mapping( + target_model: type[ModelT], + source: Mapping[str, object], + *, + origin: str, + precedence: int, + ) -> CompiledConfigPatch[ModelT]: + _require_model_type(target_model) + assignments = _mapping_assignments(target_model, source, origin=origin, precedence=precedence) + return CompiledConfigPatch.from_paths(target_model, assignments) + + @staticmethod + def from_model( + target_model: type[ModelT], source: ModelT, *, origin: str, precedence: int + ) -> CompiledConfigPatch[ModelT]: + _require_exact_model(target_model, source) + return CompiledConfigPatch.from_mapping( + target_model, + _extract_set_fields(source), + origin=origin, + precedence=precedence, + ) + + def combine(self, other: CompiledConfigPatch[ModelT]) -> CompiledConfigPatch[ModelT]: + if self.target_model is not other.target_model: + raise TypeError( + f"Cannot combine patches with different target models: " + f"{self.target_model.__name__} and {other.target_model.__name__}." + ) + return CompiledConfigPatch.from_paths(self.target_model, (*self.assignments, *other.assignments)) + + def materialize(self) -> dict[str, object]: + result: dict[str, object] = {} + ordered = sorted( + self.assignments, + key=lambda item: (item.precedence, len(item.path.parts), item.path.parts, item.origin), + ) + for assignment in ordered: + _insert_value(result, self.target_model, assignment.path.parts, deepcopy(assignment.value)) + return result + + def apply(self, base: ModelT | None = None) -> ModelT: + values: dict[str, object] = {} + if base is not None: + _require_exact_model(self.target_model, base) + values = _extract_set_fields(base) + _merge_model_mapping(values, self.target_model, self.materialize()) + return self.target_model.model_validate(values) + + +def _require_model_type(model_type: type[BaseModel]) -> None: + if not isinstance(model_type, type) or not issubclass(model_type, BaseModel): + raise TypeError(f"Expected a Pydantic target model, received {model_type!r}.") + + +def _require_exact_model(model_type: type[ModelT], value: BaseModel) -> None: + if type(value) is not model_type: + raise TypeError(f"Patch target model is {model_type.__name__}; received {type(value).__name__}.") + + +def _unwrap_annotation(annotation: object) -> object: + while get_origin(annotation) is Annotated: + annotation = get_args(annotation)[0] + origin = get_origin(annotation) + if origin not in (types.UnionType, Union): + return annotation + members = tuple(_unwrap_annotation(item) for item in get_args(annotation) if item is not type(None)) + return members[0] if len(members) == 1 else annotation + + +def _nested_model_type(annotation: object) -> type[BaseModel] | None: + annotation = _unwrap_annotation(annotation) + if isinstance(annotation, type) and issubclass(annotation, BaseModel): + return annotation + return None + + +def _field_model_at_path(model_type: type[BaseModel], path: ParameterPath) -> type[BaseModel] | None: + current = model_type + for index, part in enumerate(path.parts): + field = current.model_fields.get(part) + if field is None: + raise ParameterError(f"Unknown configuration path {str(path)!r}.") + nested = _nested_model_type(field.annotation) + if index == len(path.parts) - 1: + return nested + if nested is None: + prefix = ".".join(path.parts[: index + 1]) + raise ParameterError(f"Configuration path {str(path)!r} descends through atomic field {prefix!r}.") + current = nested + raise AssertionError("ParameterPath guarantees at least one path segment.") + + +def _mapping_assignments( + model_type: type[BaseModel], + source: Mapping[str, object], + *, + origin: str, + precedence: int, + prefix: tuple[str, ...] = (), +) -> tuple[PatchAssignment, ...]: + assignments: list[PatchAssignment] = [] + for name, value in source.items(): + # Raw mappings retain Pydantic's extra-ignore adapter contract. Canonical + # assignments use from_paths, which validates every resolved path. + if name not in model_type.model_fields: + continue + path = ParameterPath((*prefix, name)) + nested = _field_model_at_path(model_type, ParameterPath((name,))) + nested_source = _branch_mapping(nested, value) + if nested is None or nested_source is None or not nested_source: + assignments.append(PatchAssignment(path, deepcopy(value), origin, precedence)) + continue + nested_assignments = _mapping_assignments( + nested, nested_source, origin=origin, precedence=precedence, prefix=path.parts + ) + if nested_assignments: + assignments.extend(nested_assignments) + else: + # Pydantic keeps a known model branch explicit even when all of its + # provided children are ignored extras. + assignments.append(PatchAssignment(path, {}, origin, precedence)) + return tuple(assignments) + + +def _branch_mapping(nested_model: type[BaseModel] | None, value: object) -> Mapping[str, object] | None: + if nested_model is None: + return None + if isinstance(value, Mapping): + return cast(Mapping[str, object], value) + if type(value) is nested_model: + return _extract_set_fields(value) + return None + + +def _extract_set_fields(model: BaseModel) -> dict[str, object]: + extracted: dict[str, object] = {} + for name in type(model).model_fields: + value = model.__dict__[name] + if isinstance(value, BaseModel): + nested = _extract_set_fields(value) + if nested or name in model.model_fields_set: + extracted[name] = nested + continue + if name in model.model_fields_set: + extracted[name] = deepcopy(value) + return extracted + + +def _validate_conflicts(model_type: type[BaseModel], assignments: tuple[PatchAssignment, ...]) -> None: + ordered = sorted(assignments, key=lambda item: (item.path.parts, item.precedence, item.origin)) + for index, left in enumerate(ordered): + for right in ordered[index + 1 :]: + if left.path == right.path and left.precedence == right.precedence: + raise ParameterError( + f"Duplicate parameter path {str(left.path)!r} from origins {left.origin!r} and {right.origin!r}." + ) + ancestor, descendant = _ancestor_pair(left, right) + if ancestor is None or ancestor.precedence != descendant.precedence: + continue + if _branch_mapping(_field_model_at_path(model_type, ancestor.path), ancestor.value) is None: + raise ParameterError( + f"Cannot assign nested parameter path {str(descendant.path)!r}; " + f"{str(ancestor.path)!r} is already set. This parent/child conflict is between " + f"origins {descendant.origin!r} and {ancestor.origin!r}." + ) + + +def _ancestor_pair(left: PatchAssignment, right: PatchAssignment) -> tuple[PatchAssignment | None, PatchAssignment]: + if len(left.path.parts) < len(right.path.parts) and right.path.parts[: len(left.path.parts)] == left.path.parts: + return left, right + if len(right.path.parts) < len(left.path.parts) and left.path.parts[: len(right.path.parts)] == right.path.parts: + return right, left + return None, right + + +def _insert_value( + target: dict[str, object], model_type: type[BaseModel], parts: tuple[str, ...], value: object +) -> None: + name, *tail = parts + field = model_type.model_fields[name] + nested_model = _nested_model_type(field.annotation) + if tail: + if nested_model is None: + raise AssertionError("Validated paths cannot descend through atomic fields.") + branch = target.get(name) + nested = _as_object_dict(branch) + target[name] = nested + _insert_value(nested, nested_model, tuple(tail), value) + return + branch_source = _branch_mapping(nested_model, value) + if branch_source is None: + target[name] = value + return + if nested_model is None: + raise AssertionError("A branch mapping must have a nested model type.") + branch = target.get(name) + nested = _as_object_dict(branch) + target[name] = nested + _merge_model_mapping(nested, nested_model, branch_source) + + +def _as_object_dict(value: object) -> dict[str, object]: + if isinstance(value, dict): + return cast(dict[str, object], value) + return {} + + +def _merge_model_mapping(target: dict[str, object], model_type: type[BaseModel], source: Mapping[str, object]) -> None: + for name, value in source.items(): + if name not in model_type.model_fields: + raise ParameterError(f"Unknown configuration path {name!r} for {model_type.__name__}.") + _insert_value(target, model_type, (name,), deepcopy(value)) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index 4586c7c3e..fc6a67f6f 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -135,9 +135,7 @@ def resolve(self, name: str) -> ParameterNameResolution: return ResolvedParameterName(candidates[0]) -def _iter_parameter_fields( - model_type: type[Parameters], prefix: tuple[str, ...] = () -) -> tuple[ParameterField, ...]: +def _iter_parameter_fields(model_type: type[Parameters], prefix: tuple[str, ...] = ()) -> tuple[ParameterField, ...]: fields: list[ParameterField] = [] for name, field_info in model_type.model_fields.items(): path = ParameterPath((*prefix, name)) diff --git a/tests/cli/test_utils.py b/tests/cli/test_utils.py index 8fe75efec..a300d1af6 100644 --- a/tests/cli/test_utils.py +++ b/tests/cli/test_utils.py @@ -354,8 +354,8 @@ def test_apply_cli_overrides_without_registry( class TestMergeOverrides: """Tests for config-file and CLI override merging.""" - def test_partial_config_preserves_explicit_field_metadata(self, tmp_path: Path): - """Partial config files should not make default values look explicit.""" + def test_partial_config_keeps_validator_and_factory_defaults_implicit(self, tmp_path: Path): + """Partial config files preserve only explicitly supplied nested fields.""" config_file = tmp_path / "config.yaml" config_file.write_text(""" generation: @@ -366,7 +366,38 @@ def test_partial_config_preserves_explicit_field_metadata(self, tmp_path: Path): assert config.generation.num_records == 77 assert config.generation.structured_generation.enabled is False - assert config.model_dump(exclude_unset=True) == {"generation": {"num_records": 77}} + sparse = config.model_dump(exclude_unset=True) + assert sparse["generation"] == {"num_records": 77} + assert "data" not in sparse + assert "replace_pii" not in sparse + assert "evaluation" not in sparse + + @pytest.mark.parametrize( + ("config_contents", "overrides", "expected"), + [ + (None, {"unknown": True}, {}), + ( + "generation:\n num_records: 77\n", + {"generation": {"unknown": True}}, + {"generation": {"num_records": 77}}, + ), + ], + ) + def test_unknown_override_is_ignored( + self, + tmp_path: Path, + config_contents: str | None, + overrides: dict[str, object], + expected: dict[str, object], + ): + config_file = None + if config_contents is not None: + config_file = tmp_path / "config.yaml" + config_file.write_text(config_contents) + + config = merge_overrides(config_file, overrides) + + assert config.model_dump(exclude_unset=True) == expected class TestPropagateRuntimeSettingsToEnv: diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 1b3b8831c..92e9aa275 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -243,19 +243,43 @@ def test_from_config_patch_validates_sparse_config(): assert config.replace_pii is None -def test_with_config_patch_merges_sparse_patch_and_keeps_defaults_implicit(): +@pytest.mark.parametrize( + ("patch", "expected"), + [ + ({"unknown": True}, {}), + ({"generation": {"unknown": True}}, {"generation": {}}), + ], +) +def test_from_config_patch_ignores_unknown_mapping_keys(patch: dict[str, object], expected: dict[str, object]): + config = SafeSynthesizerParameters.from_config_patch(patch) + + assert config.model_dump(exclude_unset=True) == expected + + +def test_with_config_patch_ignores_unknown_mapping_keys_and_preserves_sparse_base(): + config = SafeSynthesizerParameters.model_validate({"generation": {"num_records": 77}}) + + merged = config.with_config_patch({"unknown": True, "generation": {"unknown": True, "temperature": 0.7}}) + + assert merged.model_dump(exclude_unset=True) == {"generation": {"num_records": 77, "temperature": 0.7}} + + +def test_with_config_patch_keeps_validator_and_factory_defaults_implicit(): config = SafeSynthesizerParameters.model_validate({"generation": {"num_records": 77}}) merged = config.with_config_patch({"generation": {"temperature": 0.7}, "training": {"batch_size": 4}}) assert merged.generation.num_records == 77 assert merged.generation.temperature == 0.7 - assert merged.generation.use_structured_generation is False + assert merged.generation.structured_generation.enabled is False assert merged.training.batch_size == 4 - assert merged.model_dump(exclude_unset=True) == { - "generation": {"num_records": 77, "temperature": 0.7}, - "training": {"batch_size": 4}, - } + sparse = merged.model_dump(exclude_unset=True) + assert sparse["generation"] == {"num_records": 77, "temperature": 0.7} + assert sparse["training"] == {"batch_size": 4} + assert "data" not in sparse + assert "replace_pii" not in sparse + assert "evaluation" not in sparse + assert "time_series" not in sparse class _LeftParameters(Parameters): diff --git a/tests/config/test_patch.py b/tests/config/test_patch.py new file mode 100644 index 000000000..93bf3f25d --- /dev/null +++ b/tests/config/test_patch.py @@ -0,0 +1,233 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from copy import deepcopy +from typing import Self + +import pytest +from pydantic import BaseModel, Field, model_validator + +from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters +from nemo_safe_synthesizer.config.patch import CompiledConfigPatch, PatchAssignment +from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig, StepDefinition +from nemo_safe_synthesizer.configurator.parameter_paths import ParameterPath, ParameterSchema, UnknownParameterName +from nemo_safe_synthesizer.errors import ParameterError + + +class _Child(BaseModel): + count: int = 3 + label: str = "default" + + +class _PatchTarget(BaseModel): + child: _Child | None = Field(default_factory=_Child) + payload: dict[str, object] | None = None + items: list[dict[str, int]] = Field(default_factory=list) + validated_count: int = 0 + + @model_validator(mode="after") + def record_top_level_validation(self) -> Self: + object.__setattr__(self, "validated_count", self.validated_count + 1) + return self + + +class _OtherTarget(BaseModel): + child: _Child | None = None + + +def _assignment(path: str, value: object, *, origin: str = "test", precedence: int = 0) -> PatchAssignment: + return PatchAssignment(ParameterPath(tuple(path.split("."))), value, origin, precedence) + + +def _paths(*assignments: PatchAssignment) -> CompiledConfigPatch[_PatchTarget]: + return CompiledConfigPatch.from_paths(_PatchTarget, assignments) + + +def test_mapping_leaf_with_nested_dictionaries_is_atomic_and_isolated() -> None: + fallback = {"fallback": "name"} + source = {"vars": {"template": {"given": ["first", fallback]}}} + patch = CompiledConfigPatch.from_mapping(StepDefinition, source, origin="mapping", precedence=0) + fallback["fallback"] = "changed" + + first = patch.apply() + second = patch.apply() + assert first.vars == {"template": {"given": ["first", {"fallback": "name"}]}} + + first.vars["template"]["given"][1]["fallback"] = "result-change" # type: ignore[index] + assert second.vars == {"template": {"given": ["first", {"fallback": "name"}]}} + + +def test_nested_nss_model_branch_patch_preserves_pii_global_siblings() -> None: + base = PiiReplacerConfig.get_default_config() + original_entities = deepcopy(base.globals.classify.entities) + patch = CompiledConfigPatch.from_mapping( + PiiReplacerConfig, {"globals": {"seed": 17}}, origin="override", precedence=1 + ) + + result = patch.apply(base) + + assert result.globals.seed == 17 + assert result.globals.classify.entities == original_entities + assert result.steps == base.steps + + +@pytest.mark.parametrize("path", ["payload.nested", "items.0", "child.count.value"]) +def test_mapping_collection_and_scalar_leaves_reject_descendants(path: str) -> None: + with pytest.raises(ParameterError, match=path.rsplit(".", 1)[0]): + _paths(_assignment(path, "invalid")) + + +@pytest.mark.parametrize("reverse", [False, True], ids=["parent-first", "child-first"]) +@pytest.mark.parametrize("parent", [{"label": "mapping"}, _Child(label="model")], ids=["mapping", "model"]) +def test_compatible_branch_parent_and_child_merge_in_both_orders(parent: object, reverse: bool) -> None: + assignments = [_assignment("child", parent), _assignment("child.count", 9)] + if reverse: + assignments.reverse() + + result = _paths(*assignments).apply() + + assert result.child is not None + assert result.child.count == 9 + assert result.child.label in {"mapping", "model"} + + +@pytest.mark.parametrize("parent", [None, 4], ids=["none", "scalar"]) +@pytest.mark.parametrize("reverse", [False, True], ids=["parent-first", "child-first"]) +def test_atomic_ancestor_conflicts_are_input_order_independent(parent: object, reverse: bool) -> None: + assignments = [_assignment("child", parent, origin="parent"), _assignment("child.count", 9, origin="child")] + if reverse: + assignments.reverse() + + with pytest.raises(ParameterError, match=r"parent/child.*child.*parent"): + _paths(*assignments) + + +@pytest.mark.parametrize("reverse", [False, True], ids=["first-second", "second-first"]) +def test_exact_duplicate_diagnostic_includes_path_and_origins_independent_of_order(reverse: bool) -> None: + assignments = [ + _assignment("child.count", 4, origin="first"), + _assignment("child.count", 5, origin="second"), + ] + if reverse: + assignments.reverse() + + with pytest.raises(ParameterError, match=r"(?i)duplicate.*child\.count.*first.*second"): + _paths(*assignments) + + +def test_higher_precedence_child_replaces_lower_atomic_parent() -> None: + patch = _paths( + _assignment("child", None, origin="file", precedence=0), + _assignment("child.count", 11, origin="cli", precedence=1), + ) + + result = patch.apply() + + assert result.child is not None + assert result.child.count == 11 + + +def test_higher_precedence_atomic_parent_replaces_lower_child() -> None: + patch = _paths( + _assignment("child.count", 11, origin="file", precedence=0), + _assignment("child", None, origin="cli", precedence=1), + ) + + assert patch.apply().child is None + + +def test_higher_precedence_parent_seed_wins_only_its_overlapping_children() -> None: + patch = _paths( + _assignment("child.count", 4, origin="file", precedence=0), + _assignment("child.label", "file-label", origin="file", precedence=0), + _assignment("child", {"count": 8}, origin="cli", precedence=1), + ) + + result = patch.apply() + + assert result.child == _Child(count=8, label="file-label") + + +def test_absence_explicit_none_and_explicit_default_remain_distinct() -> None: + absent = CompiledConfigPatch.from_mapping(_PatchTarget, {}, origin="empty", precedence=0).apply() + explicit_none = _paths(_assignment("child", None)).apply() + explicit_default = _paths(_assignment("child.count", 3)).apply() + + assert "child" not in absent.model_fields_set + assert explicit_none.child is None + assert explicit_none.model_fields_set == {"child"} + assert explicit_default.child is not None + assert explicit_default.child.model_fields_set == {"count"} + + +def test_mapping_constructor_ignores_unknown_keys_at_each_model_level() -> None: + patch = CompiledConfigPatch.from_mapping( + _PatchTarget, + {"unknown": True, "child": {"unknown": True}}, + origin="mapping", + precedence=0, + ) + + result = patch.apply() + + assert result.model_dump(exclude_unset=True) == {"child": {}} + + +def test_path_constructor_remains_strict_for_unknown_canonical_path() -> None: + with pytest.raises(ParameterError, match=r"Unknown configuration path 'unknown'"): + _paths(_assignment("unknown", True)) + + +def test_model_constructor_recursively_extracts_sparse_explicit_fields() -> None: + source = _PatchTarget() + assert source.model_fields_set == set() + assert source.child is not None + source.child.count = 12 + + patch = CompiledConfigPatch.from_model(_PatchTarget, source, origin="base", precedence=0) + result = patch.apply() + + assert result.child is not None + assert result.child.count == 12 + assert result.model_dump(exclude_unset=True) == {"child": {"count": 12}} + + +def test_model_source_and_results_do_not_share_mutable_payloads() -> None: + source = _PatchTarget(items=[{"value": 1}]) + patch = CompiledConfigPatch.from_model(_PatchTarget, source, origin="model", precedence=0) + source.items[0]["value"] = 2 + + first = patch.apply() + second = patch.apply() + first.items[0]["value"] = 3 + + assert second.items == [{"value": 1}] + + +def test_top_level_validator_runs_at_application_boundary() -> None: + result = _paths(_assignment("child.count", 6)).apply() + + assert result.validated_count == 1 + + +def test_wrong_target_model_is_rejected_for_combine_and_apply() -> None: + patch = _paths(_assignment("child.count", 6)) + other = CompiledConfigPatch.from_mapping(_OtherTarget, {}, origin="other", precedence=0) + + with pytest.raises(TypeError, match="target model"): + patch.combine(other) # ty: ignore[invalid-argument-type] -- runtime rejection is the contract + with pytest.raises(TypeError, match="target model"): + patch.apply(_OtherTarget()) # ty: ignore[invalid-argument-type] -- runtime rejection is the contract + + +def test_patch_schema_does_not_widen_public_pii_name_resolution() -> None: + CompiledConfigPatch.from_mapping( + SafeSynthesizerParameters, + {"replace_pii": {"globals": {"seed": 3}}}, + origin="config", + precedence=0, + ) + + assert isinstance( + ParameterSchema.from_model(SafeSynthesizerParameters).resolve("replace_pii.globals.seed"), UnknownParameterName + ) From 4ecb92e43cb6cc9ea7aa124faf72bc79b49247ae Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 01:29:02 +0000 Subject: [PATCH 11/19] refactor(config): centralize model config normalization Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 63 ++--- src/nemo_safe_synthesizer/config/patch.py | 51 +++++ .../configurator/parameters.py | 55 ++++- .../sdk/config_builder.py | 123 ++-------- tests/config/test_parameters.py | 216 +++++++++++++++++- tests/sdk/test_config_builder.py | 135 ++++++++++- 6 files changed, 490 insertions(+), 153 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 66b96458b..971f7dfa1 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -7,7 +7,7 @@ from collections.abc import Mapping from typing import Self, TypeAlias -from pydantic import BaseModel, Field, model_validator +from pydantic import Field, model_validator from typing_extensions import override from ..configurator.parameter_paths import ( @@ -21,7 +21,6 @@ from ..errors import ParameterError from ..observability import get_logger from ..telemetry import _telemetry_enabled -from ..utils import merge_dicts from .data import DataParameters from .differential_privacy import DifferentialPrivacyHyperparams from .evaluate import EvaluationParameters @@ -34,7 +33,6 @@ from .types import AUTO_STR ConfigPatch: TypeAlias = Mapping[str, object] -_SectionPatch: TypeAlias = dict[str, object] __all__ = ["ConfigPatch", "SafeSynthesizerParameters"] @@ -42,42 +40,6 @@ logger = get_logger(__name__) -def _collect_set_fields(model: BaseModel) -> _SectionPatch: - """Recursively collect a model's explicitly-set fields as a nested dict. - - Unlike ``model_dump(exclude_unset=True)``, nested models are always - traversed -- even when the parent did not mark the nested field as set -- - so in-place mutations of nested fields (e.g. - ``cfg.generation.validation.foo = True``) are captured. A nested model is - included only when it has at least one set field of its own. - """ - overrides: _SectionPatch = {} - for name in type(model).model_fields: - value = getattr(model, name) - if isinstance(value, BaseModel): - nested = _collect_set_fields(value) - if nested: - overrides[name] = nested - elif name in model.__pydantic_fields_set__: - overrides[name] = value - return overrides - - -def _overlay_set_fields(saved: Parameters, runtime: Parameters) -> Parameters: - """Deep-merge ``runtime``'s explicitly-set fields onto ``saved``. - - Only fields ``runtime`` marks as set (recursively, at every nesting level) - override ``saved``; unset fields keep their saved values. The merged mapping - is revalidated through the model so nested groups and type coercion are - handled correctly. Returns ``saved`` unchanged when ``runtime`` sets no - fields. - """ - overrides = _collect_set_fields(runtime) - if not overrides: - return saved - return saved.model_validate(merge_dicts(saved.model_dump(), overrides)) - - _LEGACY_FLAT_PATHS: dict[str, tuple[str, ...]] = { "use_structured_generation": ("generation", "structured_generation", "enabled"), "structured_generation_backend": ("generation", "structured_generation", "backend"), @@ -306,17 +268,18 @@ def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSyn not affect the other. """ updates: dict[str, object] = {} - # Only record sections that actually changed; unchanged sections are - # deep-copied by ``model_copy(deep=True)`` below so the returned config - # never shares mutable sub-objects with ``self``. - generation = _overlay_set_fields(self.generation, runtime.generation) - if generation is not self.generation: + generation = runtime.generation.explicit_patch().materialize() + if generation or "generation" in runtime.model_fields_set: updates["generation"] = generation - evaluation = _overlay_set_fields(self.evaluation, runtime.evaluation) - if evaluation is not self.evaluation: + evaluation = runtime.evaluation.explicit_patch().materialize() + if evaluation or "evaluation" in runtime.model_fields_set: updates["evaluation"] = evaluation - # emit_telemetry is a top-level scalar: detect explicit assignment, - # since there is no sub-model to inspect for set fields. - if "emit_telemetry" in runtime.__pydantic_fields_set__: + if "emit_telemetry" in runtime.model_fields_set: updates["emit_telemetry"] = runtime.emit_telemetry - return self.model_copy(update=updates, deep=True) + patch = CompiledConfigPatch.from_mapping( + type(self), + updates, + origin="runtime override", + precedence=1, + ) + return self.apply_patch(patch) diff --git a/src/nemo_safe_synthesizer/config/patch.py b/src/nemo_safe_synthesizer/config/patch.py index 11e96d493..4a52cf0bc 100644 --- a/src/nemo_safe_synthesizer/config/patch.py +++ b/src/nemo_safe_synthesizer/config/patch.py @@ -97,6 +97,16 @@ def apply(self, base: ModelT | None = None) -> ModelT: _merge_model_mapping(values, self.target_model, self.materialize()) return self.target_model.model_validate(values) + def _apply_to_full_model(self, base: ModelT) -> ModelT: + """Apply to current full values while retaining sparse field presence.""" + _require_exact_model(self.target_model, base) + patch_values = self.materialize() + values = base.model_dump() + _merge_model_mapping(values, self.target_model, patch_values) + result = self.target_model.model_validate(values) + _restore_model_fields_set(result, base, patch_values) + return result + def _require_model_type(model_type: type[BaseModel]) -> None: if not isinstance(model_type, type) or not issubclass(model_type, BaseModel): @@ -197,6 +207,47 @@ def _extract_set_fields(model: BaseModel) -> dict[str, object]: return extracted +def _restore_model_fields_set(result: BaseModel, base: BaseModel, patch: Mapping[str, object]) -> None: + """Restore recursive base presence and add fields supplied by ``patch``.""" + object.__setattr__(result, "__pydantic_fields_set__", set(base.model_fields_set)) + for name in type(result).model_fields: + result_value = result.__dict__[name] + if not isinstance(result_value, BaseModel): + continue + base_value = base.__dict__[name] + if isinstance(base_value, BaseModel): + _restore_model_fields_set(result_value, base_value, {}) + else: + _clear_model_fields_set(result_value) + + result.__pydantic_fields_set__.update(patch) + for name, value in patch.items(): + result_value = result.__dict__[name] + if not isinstance(result_value, BaseModel): + continue + nested_patch = _branch_mapping(type(result_value), value) + if nested_patch is not None: + _add_model_fields_set(result_value, nested_patch) + + +def _clear_model_fields_set(model: BaseModel) -> None: + object.__setattr__(model, "__pydantic_fields_set__", set()) + for value in model.__dict__.values(): + if isinstance(value, BaseModel): + _clear_model_fields_set(value) + + +def _add_model_fields_set(model: BaseModel, patch: Mapping[str, object]) -> None: + model.__pydantic_fields_set__.update(patch) + for name, value in patch.items(): + model_value = model.__dict__[name] + if not isinstance(model_value, BaseModel): + continue + nested_patch = _branch_mapping(type(model_value), value) + if nested_patch is not None: + _add_model_fields_set(model_value, nested_patch) + + def _validate_conflicts(model_type: type[BaseModel], assignments: tuple[PatchAssignment, ...]) -> None: ordered = sorted(assignments, key=lambda item: (item.path.parts, item.precedence, item.origin)) for index, left in enumerate(ordered): diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index f5e1bd357..d15f98e08 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -21,7 +21,7 @@ from abc import ABCMeta from collections.abc import Generator, Iterator, Mapping from pathlib import Path -from typing import Any, Self, get_args +from typing import Any, Self, cast, get_args import yaml from pydantic import ( @@ -31,6 +31,7 @@ from ..config.base import ( pydantic_model_config, ) +from ..config.patch import CompiledConfigPatch from ..errors import ParameterError from .parameter import ( DataT, @@ -52,6 +53,58 @@ class Parameters(BaseModel, metaclass=ABCMeta): model_config = pydantic_model_config + def explicit_patch(self) -> CompiledConfigPatch[Self]: + """Compile this model's recursively explicit fields as a sparse patch. + + Explicit fields inside nested default models are included even when the + parent field itself was never assigned. Patch values are deep-copied by + the compiler, so later mutations cannot affect patch application. + """ + model_type = type(self) + return CompiledConfigPatch.from_model(model_type, self, origin="typed config", precedence=0) + + def apply_patch(self, patch: CompiledConfigPatch[Self]) -> Self: + """Overlay a compiled patch on this full model and validate once. + + The base is materialized in full so environment-backed and validator- + resolved defaults keep their current values. Patch assignments retain + their relative precedence and always follow the base. + """ + return patch._apply_to_full_model(self) + + @classmethod + def from_config_source(cls, source: Self | Mapping[str, object] | None = None, **kwargs: object) -> Self: + """Normalize one sparse config source plus higher-precedence keyword values. + + ``source`` may be ``None``, an instance of exactly ``cls``, or a raw + mapping. Unknown mapping keys retain Pydantic's extra-ignore behavior. + A different Pydantic model type is rejected rather than adapted. + """ + match source: + case None: + source_patch = CompiledConfigPatch.from_mapping(cls, {}, origin="empty config", precedence=0) + case BaseModel() as model: + if type(model) is not cls: + raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") + source_patch = CompiledConfigPatch.from_model( + cls, + cast(Self, model), + origin="typed config", + precedence=0, + ) + case Mapping() as mapping: + source_patch = CompiledConfigPatch.from_mapping( + cls, + cast(Mapping[str, object], mapping), + origin="mapping config", + precedence=0, + ) + case _: + raise TypeError(f"Unsupported config type: {type(source)}") + + overrides = CompiledConfigPatch.from_mapping(cls, kwargs, origin="keyword override", precedence=1) + return source_patch.combine(overrides).apply() + def _isparams(self): """Marker method used by ``__subclasshook__`` to identify ``Parameters`` subclasses.""" return True diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index 6b8d2bf32..a3f19353e 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -6,10 +6,9 @@ from __future__ import annotations from collections.abc import Mapping -from typing import Self, TypeAlias, TypeVar, overload +from typing import Self, TypeAlias import pandas as pd -from pydantic import BaseModel from ..config import ( DataParameters, @@ -23,23 +22,10 @@ ) from ..observability import get_logger from ..telemetry import _telemetry_enabled -from ..utils import merge_dicts logger = get_logger(__name__) -NSSParameters = ( - DataParameters - | EvaluationParameters - | GenerateParameters - | DifferentialPrivacyHyperparams - | TimeSeriesParameters - | TrainingHyperparams - | SafeSynthesizerParameters - | PiiReplacerConfig -) - -ParamT = TypeVar("ParamT", bound=BaseModel) DataSource = pd.DataFrame | str RawConfig: TypeAlias = Mapping[str, object] ParamDict: TypeAlias = RawConfig @@ -67,7 +53,7 @@ class ConfigBuilder(object): """ def __init__(self, config: SafeSynthesizerParameters | None = None) -> None: - self._nss_config: SafeSynthesizerParameters | None = config + self._nss_config = config.model_copy(deep=True) if config is not None else None if self._nss_config is not None: self._emit_telemetry_config = self._nss_config.emit_telemetry self._evaluation_config = self._nss_config.evaluation @@ -90,53 +76,6 @@ def __init__(self, config: SafeSynthesizerParameters | None = None) -> None: self._data_source: DataSource | None = None self._classify_model_provider: str | None = None self._hf_token_secret: str | None = None - self._nss_inputs: list[str] = [ - "_data_config", - "_evaluation_config", - "_generation_config", - "_replace_pii_config", - "_privacy_config", - "_training_config", - "_time_series_config", - ] - - @overload - def _resolve_config(self, values: ParamT, cls: type[ParamT], **kwargs: object) -> ParamT: ... - - @overload - def _resolve_config(self, values: RawConfig, cls: type[ParamT], **kwargs: object) -> ParamT: ... - - @overload - def _resolve_config(self, values: None, cls: type[ParamT], **kwargs: object) -> ParamT: ... - - def _resolve_config(self, values: object, cls: type[ParamT], **kwargs: object) -> ParamT: - """Resolve configuration from various input types. - - Precedence: ``kwargs`` override ``values``; ``values`` override - model defaults. - - Args: - values: Existing config, a raw mapping, or ``None`` for - defaults-only. - cls: The Pydantic model class to validate against. - **kwargs: Field-level overrides applied on top. - - Returns: - A validated config instance of type ``cls``. - """ - match values: - case None: - return cls.model_validate(kwargs) - case BaseModel() as model: - if not isinstance(model, cls): - raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") - raw_values = model.model_dump(exclude_unset=True) - return cls.model_validate(merge_dicts(raw_values, kwargs)) - case Mapping() as mapping: - raw_values = dict(mapping) - return cls.model_validate(merge_dicts(raw_values, kwargs)) - case _: - raise TypeError(f"Unsupported config type: {type(values)}") def with_data_source(self, df_source: DataSource) -> Self: """Set the data source for synthetic data generation. @@ -160,7 +99,7 @@ def with_data(self, config: DataParameters | RawConfig | None = None, **kwargs: Returns: This builder instance with data processing settings applied. """ - self._data_config = self._resolve_config(values=config, cls=DataParameters, **kwargs) + self._data_config = DataParameters.from_config_source(config, **kwargs) return self def with_train(self, config: TrainingHyperparams | RawConfig | None = None, **kwargs: object) -> Self: @@ -173,9 +112,7 @@ def with_train(self, config: TrainingHyperparams | RawConfig | None = None, **kw Returns: This builder instance with training hyperparameters applied. """ - self._training_config: TrainingHyperparams | None = self._resolve_config( - values=config, cls=TrainingHyperparams, **kwargs - ) + self._training_config = TrainingHyperparams.from_config_source(config, **kwargs) return self def with_generate(self, config: GenerateParameters | RawConfig | None = None, **kwargs: object) -> Self: @@ -188,9 +125,7 @@ def with_generate(self, config: GenerateParameters | RawConfig | None = None, ** Returns: This builder instance with generation settings applied. """ - self._generation_config: GenerateParameters | None = self._resolve_config( - values=config, cls=GenerateParameters, **kwargs - ) + self._generation_config = GenerateParameters.from_config_source(config, **kwargs) return self def with_time_series(self, config: TimeSeriesParameters | RawConfig | None = None, **kwargs: object) -> Self: @@ -203,7 +138,7 @@ def with_time_series(self, config: TimeSeriesParameters | RawConfig | None = Non Returns: This builder instance with time-series synthesis settings applied. """ - self._time_series_config = self._resolve_config(values=config, cls=TimeSeriesParameters, **kwargs) + self._time_series_config = TimeSeriesParameters.from_config_source(config, **kwargs) return self def with_differential_privacy( @@ -218,7 +153,7 @@ def with_differential_privacy( Returns: This builder instance with differential privacy settings applied. """ - self._privacy_config = self._resolve_config(values=config, cls=DifferentialPrivacyHyperparams, **kwargs) + self._privacy_config = DifferentialPrivacyHyperparams.from_config_source(config, **kwargs) return self def with_replace_pii( @@ -264,13 +199,9 @@ def with_replace_pii( match config: case PiiReplacerConfig() | Mapping() as values: - cfg = self._resolve_config(values=values, cls=PiiReplacerConfig, **kwargs) + cfg = PiiReplacerConfig.from_config_source(values, **kwargs) case None: - cfg = self._resolve_config( - values=PiiReplacerConfig.get_default_config(), - cls=PiiReplacerConfig, - **kwargs, - ) + cfg = PiiReplacerConfig.from_config_source(PiiReplacerConfig.get_default_config(), **kwargs) case _: raise ValueError(f"Config must be a PiiReplacerConfig, raw mapping, or None, got {config!r}") @@ -287,7 +218,7 @@ def with_evaluate(self, config: EvaluationParameters | RawConfig | None = None, Returns: This builder instance with evaluation settings applied. """ - self._evaluation_config = self._resolve_config(values=config, cls=EvaluationParameters, **kwargs) + self._evaluation_config = EvaluationParameters.from_config_source(config, **kwargs) return self def resolve(self) -> Self: @@ -307,28 +238,20 @@ def resolve(self) -> Self: def _resolve_nss_config(self) -> None: """Assemble per-section configs into a ``SafeSynthesizerParameters``. - Iterates over ``_nss_inputs``, maps each ``_*_config`` attribute - to its ``SafeSynthesizerParameters`` field name, and constructs - the unified config. Also injects ``_classify_model_provider`` - into the PII replacer config when set. + Constructs the unified config from already-normalized typed sections, + then injects ``_classify_model_provider`` into PII configuration when + requested. """ - params_map: dict = {k: k.split("_")[1] for k in self._nss_inputs} - params_map["_replace_pii_config"] = "replace_pii" - params_map["_time_series_config"] = "time_series" - params_to_use: dict = {k: None for k in params_map.values()} - - for pg, name in params_map.items(): - param: NSSParameters | None = getattr(self, pg, None) - match param: - case BaseModel() as c: - params_to_use[name] = c - case dict() as d: - params_to_use[name] = d - case None: - logger.debug(f"Using default values for {pg}") - case _: - raise ValueError(f"Input must be a BaseModel, dictionary, or None: {type(param)}") - self._nss_config = SafeSynthesizerParameters(**params_to_use, emit_telemetry=self._emit_telemetry_config) + self._nss_config = SafeSynthesizerParameters( + data=self._data_config, + evaluation=self._evaluation_config, + training=self._training_config, + generation=self._generation_config, + privacy=self._privacy_config, + time_series=self._time_series_config, + replace_pii=self._replace_pii_config, + emit_telemetry=self._emit_telemetry_config, + ) # Inject classify_model_provider into PII replacer config if set if self._classify_model_provider and self._nss_config.replace_pii: diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 92e9aa275..ef01d225b 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -2,11 +2,13 @@ # SPDX-License-Identifier: Apache-2.0 import warnings +from collections.abc import Mapping from pathlib import Path +from typing import ClassVar, cast from unittest.mock import MagicMock, patch import pytest -from pydantic import Field, ValidationError +from pydantic import Field, ValidationError, model_validator from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig, StepDefinition @@ -295,6 +297,154 @@ class _DuplicateLeafParameters(Parameters): right: _RightParameters = Field(default_factory=_RightParameters) +class _PresenceParameters(Parameters): + left: _LeftParameters = Field(default_factory=_LeftParameters) + right: _RightParameters = Field(default_factory=_RightParameters) + optional: _LeftParameters | None = Field(default_factory=_LeftParameters) + + +class _MappingParameters(Parameters): + payload: dict[str, object] = Field(default_factory=dict) + + +class _OrderedParameters(Parameters): + low: int = 1 + high: int = 2 + + @model_validator(mode="after") + def validate_order(self): + if self.low >= self.high: + raise ValueError("low must be less than high") + return self + + +class _ValidatedSafeSynthesizerParameters(SafeSynthesizerParameters): + validation_runs: ClassVar[int] = 0 + + @model_validator(mode="after") + def record_validation(self): + type(self).validation_runs += 1 + return self + + +def test_explicit_patch_captures_sparse_nested_in_place_mutation(): + source = _DuplicateLeafParameters() + source.left.value = 17 + + result = source.explicit_patch().apply() + + assert result.model_dump(exclude_unset=True) == {"left": {"value": 17}} + + +@pytest.mark.parametrize( + ("source", "expected_fields"), + [ + pytest.param(None, set(), id="none"), + pytest.param({"value": 5, "ignored": True}, {"value"}, id="mapping"), + pytest.param(_LeftParameters(value=7), {"value"}, id="typed"), + ], +) +def test_from_config_source_normalizes_supported_sources( + source: _LeftParameters | Mapping[str, object] | None, expected_fields: set[str] +): + result = _LeftParameters.from_config_source(source) + + assert result.model_fields_set == expected_fields + + +def test_from_config_source_rejects_wrong_exact_model_type(): + with pytest.raises(TypeError, match=r"Expected _LeftParameters, got _RightParameters"): + _LeftParameters.from_config_source(_RightParameters()) # ty: ignore[invalid-argument-type] + + +def test_from_config_source_kwargs_override_source_and_preserve_nested_siblings(): + result = _DuplicateLeafParameters.from_config_source( + {"left": {"value": 4}, "right": {"value": 6}}, + left={"value": 9}, + ) + + assert result.left.value == 9 + assert result.right.value == 6 + + +def test_from_config_source_copies_mapping_and_returned_mutable_state(): + nested = {"items": [1]} + source = {"payload": nested} + + result = _MappingParameters.from_config_source(source) + nested["items"].append(2) # type: ignore[union-attr] + cast(list[int], result.payload["items"]).append(3) + + assert source == {"payload": {"items": [1, 2]}} + + +def test_from_config_source_copies_typed_source_state(): + source = _MappingParameters(payload={"items": [1]}) + + result = _MappingParameters.from_config_source(source) + cast(list[int], result.payload["items"]).append(2) + + assert source.payload == {"items": [1]} + + +def test_mapping_valued_atomic_leaf_is_not_parsed_as_model_branch(): + result = _MappingParameters.from_config_source({"payload": {"unknown": {"nested": True}}}) + + assert result.payload == {"unknown": {"nested": True}} + + +def test_apply_patch_preserves_sparse_base_and_runs_validation(): + base = _OrderedParameters(low=3, high=5) + patch = _OrderedParameters.from_config_source({"high": 4}).explicit_patch() + + result = base.apply_patch(patch) + + assert result.model_dump(exclude_unset=True) == {"low": 3, "high": 4} + with pytest.raises(ValidationError, match="low must be less than high"): + base.apply_patch(_OrderedParameters.from_config_source({"high": 2}).explicit_patch()) + + +def test_apply_empty_patch_preserves_recursive_explicit_fields(): + base = _PresenceParameters() + base.left.value = 17 + + result = base.apply_patch(_PresenceParameters().explicit_patch()) + + assert result.left.value == 17 + assert result.model_fields_set == set() + assert result.left.model_fields_set == {"value"} + assert result.right.model_fields_set == set() + assert result.optional is not None + assert result.optional.model_fields_set == set() + assert result.model_dump(exclude_unset=True) == {} + + +def test_apply_nonempty_patch_adds_only_patch_explicit_fields(): + base = _PresenceParameters.from_config_source({"left": {}}) + patch = _PresenceParameters.from_config_source( + {"left": {"value": 1}, "right": {}, "optional": None} + ).explicit_patch() + + result = base.apply_patch(patch) + + assert result.model_fields_set == {"left", "right", "optional"} + assert result.left.model_fields_set == {"value"} + assert result.right.model_fields_set == set() + assert result.optional is None + assert result.model_dump(exclude_unset=True) == { + "left": {"value": 1}, + "right": {}, + "optional": None, + } + + +def test_apply_patch_rejects_wrong_exact_target_model(): + with pytest.raises(TypeError, match=r"target model is _RightParameters.*_LeftParameters"): + _LeftParameters().apply_patch( + _RightParameters(value=3).explicit_patch() # ty: ignore[invalid-argument-type] -- runtime rejection tested + ) + + def test_parameters_get_supports_explicit_dotted_paths(): params = _DuplicateLeafParameters() @@ -434,6 +584,70 @@ def test_does_not_mutate_saved(self): saved.with_runtime_overrides(_runtime_num_records()) assert saved.generation.num_records == 3000 + def test_ignores_explicit_non_runtime_sections(self): + saved = _saved_config() + runtime = SafeSynthesizerParameters() + runtime.training.batch_size = 99 + assert runtime.privacy is not None + runtime.privacy.dp_enabled = True + + merged = saved.with_runtime_overrides(runtime) + + assert merged.training.batch_size == 8 + assert merged.privacy == saved.privacy + + def test_runs_top_level_validation_once(self): + saved = _ValidatedSafeSynthesizerParameters() + runtime = SafeSynthesizerParameters.model_validate({"generation": {"num_records": 25}}) + _ValidatedSafeSynthesizerParameters.validation_runs = 0 + + merged = saved.with_runtime_overrides(runtime) + + assert isinstance(merged, _ValidatedSafeSynthesizerParameters) + assert _ValidatedSafeSynthesizerParameters.validation_runs == 1 + + def test_preserves_saved_implicit_telemetry_when_environment_changes(self, monkeypatch): + monkeypatch.setenv("NEMO_TELEMETRY_ENABLED", "false") + saved = SafeSynthesizerParameters() + monkeypatch.setenv("NEMO_TELEMETRY_ENABLED", "true") + + merged = saved.with_runtime_overrides(SafeSynthesizerParameters()) + + assert saved.emit_telemetry is False + assert merged.emit_telemetry is False + + def test_empty_overlay_preserves_recursive_explicit_fields(self): + saved = SafeSynthesizerParameters() + saved.generation.num_records = 3000 + + merged = saved.with_runtime_overrides(SafeSynthesizerParameters()) + + assert merged.model_fields_set == saved.model_fields_set == set() + assert merged.data.max_sequences_per_example == saved.data.max_sequences_per_example == 10 + assert merged.data.model_fields_set == saved.data.model_fields_set == {"max_sequences_per_example"} + assert merged.generation.model_fields_set == saved.generation.model_fields_set == {"num_records"} + assert merged.evaluation.model_fields_set == saved.evaluation.model_fields_set == set() + assert merged.model_dump(exclude_unset=True) == saved.model_dump(exclude_unset=True) == {} + + def test_nonempty_overlay_adds_only_allowlisted_patch_fields(self): + saved = SafeSynthesizerParameters.model_validate({"training": {"batch_size": 8}, "generation": {}}) + runtime = SafeSynthesizerParameters.model_validate( + {"generation": {"num_records": 1000}, "evaluation": {}, "emit_telemetry": False} + ) + + merged = saved.with_runtime_overrides(runtime) + + assert merged.model_fields_set == {"training", "generation", "evaluation", "emit_telemetry"} + assert merged.training.model_fields_set == {"batch_size"} + assert merged.generation.model_fields_set == {"num_records"} + assert merged.evaluation.model_fields_set == set() + assert merged.model_dump(exclude_unset=True) == { + "training": {"batch_size": 8}, + "generation": {"num_records": 1000}, + "evaluation": {}, + "emit_telemetry": False, + } + def test_returned_config_is_independent_of_saved(self): """Mutating the returned config must not affect the original (no shared references).""" saved = _saved_config() diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py index 506772faa..f92a43df2 100644 --- a/tests/sdk/test_config_builder.py +++ b/tests/sdk/test_config_builder.py @@ -1,16 +1,88 @@ # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +from collections.abc import Mapping from typing import Any, cast +import pandas as pd import pytest from pydantic import ValidationError -from nemo_safe_synthesizer.config import GenerateParameters +from nemo_safe_synthesizer.config import ( + DataParameters, + DifferentialPrivacyHyperparams, + EvaluationParameters, + GenerateParameters, + TimeSeriesParameters, + TrainingHyperparams, +) from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig +from nemo_safe_synthesizer.configurator.parameters import Parameters from nemo_safe_synthesizer.sdk.config_builder import ConfigBuilder +@pytest.mark.parametrize("as_mapping", [False, True], ids=["typed", "mapping"]) +@pytest.mark.parametrize( + ("method_name", "model_type", "values", "field", "expected"), + [ + pytest.param("with_data", DataParameters, {"holdout": 0.2}, "holdout", 0.2, id="data"), + pytest.param("with_train", TrainingHyperparams, {"batch_size": 4}, "batch_size", 4, id="train"), + pytest.param("with_generate", GenerateParameters, {"num_records": 12}, "num_records", 12, id="generate"), + pytest.param( + "with_time_series", + TimeSeriesParameters, + {"is_timeseries": True, "timestamp_interval_seconds": 60}, + "is_timeseries", + True, + id="time-series", + ), + pytest.param( + "with_differential_privacy", + DifferentialPrivacyHyperparams, + {"epsilon": 3.0}, + "epsilon", + 3.0, + id="privacy", + ), + pytest.param( + "with_evaluate", EvaluationParameters, {"mia_enabled": False}, "mia_enabled", False, id="evaluate" + ), + ], +) +def test_builder_methods_accept_typed_and_mapping_sources( + as_mapping: bool, + method_name: str, + model_type: type[Parameters], + values: dict[str, object], + field: str, + expected: object, +): + source: Parameters | Mapping[str, object] = values if as_mapping else model_type.model_validate(values) + + builder = getattr(ConfigBuilder(), method_name)(config=source) + section_name = { + "with_data": "_data_config", + "with_train": "_training_config", + "with_generate": "_generation_config", + "with_time_series": "_time_series_config", + "with_differential_privacy": "_privacy_config", + "with_evaluate": "_evaluation_config", + }[method_name] + + assert getattr(getattr(builder, section_name), field) == expected + + +@pytest.mark.parametrize( + "method_name", + ["with_data", "with_train", "with_generate", "with_time_series", "with_differential_privacy", "with_evaluate"], +) +def test_builder_methods_reject_wrong_typed_model(method_name: str): + wrong = GenerateParameters() if method_name != "with_generate" else DataParameters() + + with pytest.raises(TypeError, match="Expected"): + getattr(ConfigBuilder(), method_name)(config=wrong) + + def test_with_generate_validates_raw_config_with_kwargs(): with pytest.raises(ValidationError, match="patience"): ConfigBuilder().with_generate(config={"num_records": 10}, patience=0) @@ -71,3 +143,64 @@ def test_with_replace_pii_deep_merges_nested_kwargs(as_mapping: bool): assert builder._replace_pii_config is not None assert builder._replace_pii_config.globals.locales == ["en_US"] assert builder._replace_pii_config.globals.classify.enable_classify is False + assert builder._replace_pii_config.steps[0].vars == config_model.steps[0].vars + + +def test_with_replace_pii_none_uses_defaults_and_preserves_step_vars_with_nested_kwargs(): + default = PiiReplacerConfig.get_default_config() + + builder = ConfigBuilder().with_replace_pii(globals={"classify": {"enable_classify": False}}) + + assert builder._replace_pii_config is not None + assert builder._replace_pii_config.globals.locales == default.globals.locales + assert builder._replace_pii_config.steps[0].vars == default.steps[0].vars + + +def test_with_replace_pii_invalid_source_preserves_value_error_contract(): + with pytest.raises(ValueError, match="Config must be"): + ConfigBuilder().with_replace_pii(config=GenerateParameters()) # ty: ignore[invalid-argument-type] + + +def test_with_generate_captures_nested_mutation_on_sparse_typed_source(): + source = GenerateParameters() + source.validation.group_by_fix_unordered_records = True + + builder = ConfigBuilder().with_generate(config=source) + + assert builder._generation_config.model_dump(exclude_unset=True) == { + "validation": {"group_by_fix_unordered_records": True} + } + + +def test_resolve_runs_top_level_validation_for_direct_typed_assembly(): + builder = ( + ConfigBuilder() + .with_data_source(pd.DataFrame({"value": [1]})) + .with_data(max_sequences_per_example=2) + .with_differential_privacy(dp_enabled=True) + ) + + with pytest.raises(ValidationError, match="max_sequences_per_example must be 1"): + builder.resolve() + + +def test_resolved_config_is_independent_of_mapping_source(): + entities = ["email"] + source = {"pii_replay_entities": entities} + + builder = ConfigBuilder().with_data_source(pd.DataFrame({"value": [1]})).with_evaluate(config=source).resolve() + entities.append("phone_number") + + assert builder._nss_config is not None + assert builder._nss_config.evaluation.pii_replay_entities == ["email"] + + +def test_direct_assembly_preserves_classify_model_provider_injection(): + builder = ConfigBuilder().with_data_source(pd.DataFrame({"value": [1]})) + builder._classify_model_provider = "test-provider" + + builder.resolve() + + assert builder._nss_config is not None + assert builder._nss_config.replace_pii is not None + assert builder._nss_config.replace_pii.globals.classify.classify_model_provider == "test-provider" From 0a72e0c001df233b8ad45dd6b04d1431ace423a7 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 01:54:55 +0000 Subject: [PATCH 12/19] docs(config): clarify patch semantics Signed-off-by: Aaron Gonzales --- STYLE_GUIDE.md | 16 ++--- docs/user-guide/configuration.md | 72 ++++++++++++++----- src/nemo_safe_synthesizer/cli/utils.py | 10 +-- .../config/parameters.py | 23 +++--- .../configurator/pydantic_click_options.py | 4 +- .../sdk/config_builder.py | 12 ++-- 6 files changed, 82 insertions(+), 55 deletions(-) diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md index 199986840..515931389 100644 --- a/STYLE_GUIDE.md +++ b/STYLE_GUIDE.md @@ -443,23 +443,17 @@ def teardown(self) -> None: Tier 2 -- moderate. Summary + `Args:` / `Returns:` / `Raises:` blocks: ```python -def _resolve_config(self, values: ParamDict | NSSParameters | None, cls: type[ParamT], **kwargs) -> ParamT: - """Resolve configuration from various input types. - - Merges caller-supplied overrides on top of a base config. Accepts Pydantic models - (copied with updates), plain dicts (validated then updated), or None (built from - overrides alone). +def load_config(path: Path) -> SafeSynthesizerParameters: + """Load and validate a Safe Synthesizer YAML configuration. Args: - values: Base configuration -- a Pydantic model, a dict, or None. - cls: The Pydantic model class to validate against. - **kwargs: Field-level overrides applied on top of the base. + path: YAML configuration path. Returns: - An instance of `cls` with all overrides applied. + The validated pipeline configuration. Raises: - TypeError: If `values` is not a BaseModel, dict, or None. + FileNotFoundError: If ``path`` does not exist. """ ``` diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index 0f5229354..ad7fd3b86 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -58,38 +58,78 @@ for more detail on combining config files with runtime overrides. ### Python Parameter Construction -The Python SDK accepts both fully nested config objects and compatibility -shortcuts for fields on top-level parameter sections: +`SafeSynthesizerParameters.from_params()` accepts four forms of keyword name: + +- top-level fields such as `generation` or `replace_pii`; +- canonical dotted paths such as `generation.num_records`; +- bare leaf names such as `num_records`, when that leaf name is unique in the + configuration schema; and +- legacy structured-generation aliases, retained for compatibility. + +Python syntax requires dotted names to be passed through `**` expansion: ```python from nemo_safe_synthesizer.config import SafeSynthesizerParameters config = SafeSynthesizerParameters.from_params( - num_records=2000, # generation.num_records - dp_enabled=True, # privacy.dp_enabled - structured_generation={"enabled": True}, # generation.structured_generation.enabled + generation={"temperature": 0.8}, # top-level section + num_records=2000, # unique bare leaf + **{"generation.structured_generation.enabled": True}, # dotted path ) ``` -Flat keyword arguments are matched by field name against the top-level parameter -sections. Use the nested shape for fields inside nested subobjects, especially -when a generic field name could appear in multiple places: +An ambiguous bare name raises an error and lists the accepted dotted paths. For +example, `enabled` appears in more than one section, so specify the intended +path: ```python -# Preferred: unambiguous nested form. SafeSynthesizerParameters.from_params( - structured_generation={"enabled": True}, + **{"generation.structured_generation.enabled": True} ) +``` -# Also valid: fully nested generation section. -SafeSynthesizerParameters.from_params( - generation={"structured_generation": {"enabled": True}}, -) +Legacy aliases such as `use_structured_generation` and +`structured_generation_backend` remain accepted. New code should use the +canonical nested shape or dotted path. + +### Sparse Sources and Explicit Values + +Absence, explicit `None`, and an explicit value equal to the model default are +different inputs. An absent field inherits the next lower-precedence source or +its model default. `None` is applied when the field accepts it, and an explicitly +supplied default value still counts as an override. -# Avoid: this configures evaluation.enabled, not structured generation. -SafeSynthesizerParameters.from_params(enabled=True) +SDK section methods accept a sparse model or mapping as their source. Keyword +arguments have higher precedence than that source, while omitted source fields +retain model defaults: + +```python +synthesizer.with_generate({"temperature": 0.8}, num_records=2000) ``` +A mapping is a branch only when its schema field is another Pydantic model. +Mapping-valued leaf fields, such as free-form dictionaries, are replaced as one +atomic value rather than recursively merged. + +Persistence with `exclude_unset=True` follows Pydantic's explicit-field +metadata. Sparse model sources also inspect nested explicit fields recursively, +so an in-place mutation such as +`source.validation.group_by_fix_unordered_records = True` is captured when that +model is used as a patch input. Unrelated defaults remain implicit. + +Raw mapping sources retain the established behavior of ignoring unknown extra +keys. After a name has been resolved to a canonical path, however, that path is +strict: unknown paths and paths that descend through an atomic leaf raise an +error. + +### Resume-Time Overrides + +When generation resumes from a saved training run, runtime configuration may +override only `generation`, `evaluation`, and `emit_telemetry`. Telemetry is +overridden only when the runtime input explicitly sets it. Saved `training`, +`data`, `privacy`, PII replacement, time-series, and preflight settings remain +unchanged. + --- ## Training diff --git a/src/nemo_safe_synthesizer/cli/utils.py b/src/nemo_safe_synthesizer/cli/utils.py index 6084537ca..01128ba28 100644 --- a/src/nemo_safe_synthesizer/cli/utils.py +++ b/src/nemo_safe_synthesizer/cli/utils.py @@ -433,17 +433,17 @@ def _initialize_logging_for_cli_from_settings( def merge_overrides(config_path: str | Path | None, overrides: ConfigPatch) -> SafeSynthesizerParameters: - """Merge overrides into a SafeSynthesizerParameters object. + """Apply schema-aware overrides to a ``SafeSynthesizerParameters`` object. - If config_path is None, use the overrides to create a new SafeSynthesizerParameters object. - Otherwise, merge the overrides into the config file. + If ``config_path`` is ``None``, validate the sparse overrides as a new + configuration. Otherwise, apply them on top of the loaded YAML config. Args: config_path: Path to config file (YAML) - overrides: Dictionary of override values + overrides: Sparse nested override values. Returns: - Merged SafeSynthesizerParameters + Validated parameters with the overrides applied. """ try: if config_path is None: diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 971f7dfa1..bcfd1188f 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -193,26 +193,21 @@ def check_timeseries_group_column(self) -> Self: @classmethod @override def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": - """Convert singular, flat parameters to nested structure. + """Construct parameters from resolved keyword names. - Takes a flat dictionary of parameters, where keys correspond to - attributes of the nested parameter classes, and constructs a - ``SafeSynthesizerParameters`` instance with the appropriate nested - structure, using default values for each subgroup that are not - explicitly provided. + Names may be top-level fields, canonical dotted paths, unique bare leaf + names, or supported legacy aliases. Ambiguous bare names raise an error + that lists the canonical dotted alternatives. - Args: - **kwargs: Flat key-value pairs that map to attributes of the - nested parameter classes (e.g., ``TrainingHyperparams``, - ``GenerateParameters``). + Args: + **kwargs: Values keyed by a supported parameter name. - Returns: - A fully initialized ``SafeSynthesizerParameters`` instance with - nested sub-configurations populated from the provided values. + Returns: + A validated configuration with unspecified fields defaulted. Example: >>> from nemo_safe_synthesizer.config import SafeSynthesizerParameters - >>> SafeSynthesizerParameters.from_params(structured_generation={"enabled": True}) + >>> SafeSynthesizerParameters.from_params(num_records=2000) """ schema = ParameterSchema.from_model(cls) assignments: list[PatchAssignment] = [] diff --git a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py index 5143aa5f3..ad6def640 100644 --- a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py +++ b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py @@ -54,8 +54,8 @@ def parse_overrides(values: dict[str, Any] | None = None, field_sep: str = "__") field_sep: Separator used to reconstruct nesting. For example, ``{"data__holdout": 0.1}`` becomes ``{"data": {"holdout": 0.1}}``. Returns: - A nested dict suitable for ``model_validate()`` or for merging - with a loaded config via ``merge_dicts()``. + A nested dictionary suitable for schema-aware config patching or direct + model validation. Raises: ValueError: If a key contains empty segments (e.g. consecutive diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index a3f19353e..7cbf8f17d 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -28,10 +28,9 @@ DataSource = pd.DataFrame | str RawConfig: TypeAlias = Mapping[str, object] -ParamDict: TypeAlias = RawConfig -class ConfigBuilder(object): +class ConfigBuilder: """Fluent builder for assembling Safe Synthesizer configuration. Accumulates per-section configuration objects (data, training, @@ -40,11 +39,10 @@ class ConfigBuilder(object): ``SafeSynthesizer`` do it) to collapse them into a single ``SafeSynthesizerParameters``. - Each ``with_*`` method accepts an optional typed config object or - a raw mapping, plus ``**kwargs`` overrides. ``kwargs`` always take - precedence over fields in the config/mapping. All ``with_*`` methods - return ``Self`` so subclasses preserve their concrete type through - fluent chains. + Each ``with_*`` method accepts an optional sparse typed config or raw + mapping, plus ``**kwargs`` overrides. Keyword arguments take precedence + over fields in the source. All ``with_*`` methods return ``Self`` so + subclasses preserve their concrete type through fluent chains. Args: config: Optional pre-built parameters. When supplied, the From b725fefde9be597ab9e5a2114855c199b3cae2cf Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 16:43:29 +0000 Subject: [PATCH 13/19] fix(config): address review feedback Signed-off-by: Aaron Gonzales --- docs/user-guide/configuration.md | 2 +- .../config/parameters.py | 1 + .../configurator/parameter.py | 2 +- .../configurator/parameter_paths.py | 10 ++++++++-- .../configurator/pydantic_click_options.py | 5 +++-- .../sdk/config_builder.py | 4 ++++ tests/config/test_parameters.py | 7 +++++++ .../test_pydantic_click_options.py | 20 +++++++++---------- tests/sdk/test_config_builder.py | 11 ++++++++++ 9 files changed, 46 insertions(+), 16 deletions(-) diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index ad7fd3b86..c0263997a 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -101,7 +101,7 @@ supplied default value still counts as an override. SDK section methods accept a sparse model or mapping as their source. Keyword arguments have higher precedence than that source, while omitted source fields -retain model defaults: +retain the current lower-precedence configuration values: ```python synthesizer.with_generate({"temperature": 0.8}, num_records=2000) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index bcfd1188f..f7899163b 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -62,6 +62,7 @@ def _resolve_parameter_name(schema: ParameterSchema, name: str) -> ParameterPath case AmbiguousParameterName() as ambiguous: choices = ", ".join(str(path) for path in ambiguous.candidates) raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") + raise ParameterError(f"Unexpected parameter resolution for {name!r}.") class SafeSynthesizerParameters(Parameters): diff --git a/src/nemo_safe_synthesizer/configurator/parameter.py b/src/nemo_safe_synthesizer/configurator/parameter.py index 8d26af89d..6ad22f12a 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter.py +++ b/src/nemo_safe_synthesizer/configurator/parameter.py @@ -48,7 +48,7 @@ class Parameter(Generic[DataT]): value: DataT | Sequence[DataT] | None = None @model_serializer - def ser_model(self) -> "dict[str, DataT] | DataT | Sequence[DataT] | Parameter[DataT] | None": + def ser_model(self) -> dict[str, DataT] | DataT | Sequence[DataT] | Parameter[DataT] | None: """Serialize to the bare value for Pydantic ``model_dump`` / ``model_dump_json``.""" if hasattr(self, "value"): return self.value diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index fc6a67f6f..1000d8235 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -161,12 +161,18 @@ def split_parameter_path(name: str, separator: str = ".") -> ParameterPath: def insert_parameter_value(target: dict[str, object], path: ParameterPath, value: object) -> None: """Insert a value at an already resolved path.""" current = target - for part in path.parts[:-1]: + for index, part in enumerate(path.parts[:-1]): value_at_part = current.get(part) if isinstance(value_at_part, dict): current = cast(dict[str, object], value_at_part) continue + if part in current: + prefix = ".".join(path.parts[: index + 1]) + raise ValueError(f"Conflicting override paths for {str(path)!r}: {prefix!r} already has a parent value.") nested: dict[str, object] = {} current[part] = nested current = nested - current[path.parts[-1]] = value + leaf = path.parts[-1] + if leaf in current: + raise ValueError(f"Conflicting override paths for {str(path)!r}: nested values already exist below this path.") + current[leaf] = value diff --git a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py index ad6def640..1736e8a6f 100644 --- a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py +++ b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py @@ -59,7 +59,8 @@ def parse_overrides(values: dict[str, Any] | None = None, field_sep: str = "__") Raises: ValueError: If a key contains empty segments (e.g. consecutive - separators like ``a____b``). + separators like ``a____b``), or parent and child overrides target + incompatible values. """ if not values: return {} @@ -67,7 +68,7 @@ def parse_overrides(values: dict[str, Any] | None = None, field_sep: str = "__") for k, v in values.items(): if k.startswith("no_") and isinstance(v, bool): if v: - overrides[k[3:]] = None + insert_parameter_value(overrides, split_parameter_path(k[3:], field_sep), None) continue if v is None: continue diff --git a/src/nemo_safe_synthesizer/sdk/config_builder.py b/src/nemo_safe_synthesizer/sdk/config_builder.py index 7cbf8f17d..632be7424 100644 --- a/src/nemo_safe_synthesizer/sdk/config_builder.py +++ b/src/nemo_safe_synthesizer/sdk/config_builder.py @@ -16,6 +16,7 @@ EvaluationParameters, GenerateParameters, PiiReplacerConfig, + PreflightParameters, SafeSynthesizerParameters, TimeSeriesParameters, TrainingHyperparams, @@ -56,6 +57,7 @@ def __init__(self, config: SafeSynthesizerParameters | None = None) -> None: self._emit_telemetry_config = self._nss_config.emit_telemetry self._evaluation_config = self._nss_config.evaluation self._replace_pii_config = self._nss_config.replace_pii + self._preflight_config = self._nss_config.preflight self._privacy_config: DifferentialPrivacyHyperparams | None = self._nss_config.privacy self._training_config = self._nss_config.training self._generation_config = self._nss_config.generation @@ -66,6 +68,7 @@ def __init__(self, config: SafeSynthesizerParameters | None = None) -> None: self._evaluation_config: EvaluationParameters = EvaluationParameters() self._generation_config: GenerateParameters = GenerateParameters() self._replace_pii_config: PiiReplacerConfig | None = PiiReplacerConfig.get_default_config() + self._preflight_config = PreflightParameters() self._privacy_config: DifferentialPrivacyHyperparams = DifferentialPrivacyHyperparams() self._training_config: TrainingHyperparams = TrainingHyperparams() self._time_series_config: TimeSeriesParameters = TimeSeriesParameters() @@ -248,6 +251,7 @@ def _resolve_nss_config(self) -> None: privacy=self._privacy_config, time_series=self._time_series_config, replace_pii=self._replace_pii_config, + preflight=self._preflight_config, emit_telemetry=self._emit_telemetry_config, ) diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index ef01d225b..522c5cba1 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -202,6 +202,13 @@ def test_from_params_rejects_unknown_flat_parameter(): SafeSynthesizerParameters.from_params(not_a_parameter=True) +def test_from_params_rejects_unexpected_parameter_resolution(monkeypatch): + monkeypatch.setattr(ParameterSchema, "resolve", lambda _schema, _name: object()) + + with pytest.raises(ParameterError, match="Unexpected parameter resolution for 'num_records'"): + SafeSynthesizerParameters.from_params(num_records=10) + + def test_parameter_schema_indexes_optional_branches_without_an_instance(): with patch.object(SafeSynthesizerParameters, "__init__", side_effect=AssertionError("model instantiated")): schema = ParameterSchema.from_model(SafeSynthesizerParameters) diff --git a/tests/configurator/test_pydantic_click_options.py b/tests/configurator/test_pydantic_click_options.py index 8b3999605..303227422 100644 --- a/tests/configurator/test_pydantic_click_options.py +++ b/tests/configurator/test_pydantic_click_options.py @@ -126,16 +126,16 @@ def test_parse_overrides_custom_separator_rejects_empty_segments(key: str): parse_overrides({key: "x"}, field_sep=".") -def test_parse_overrides_no_flag_then_nested_override(): - """Nested override takes precedence over --no_ flag for same field.""" - result = parse_overrides({"no_privacy": True, "privacy__epsilon": 1.0}) - assert result == {"privacy": {"epsilon": 1.0}} - - -def test_parse_overrides_nested_then_no_flag(): - """--no_ flag after nested override disables the field.""" - result = parse_overrides({"privacy__epsilon": 1.0, "no_privacy": True}) - assert result["privacy"] is None +@pytest.mark.parametrize( + "values", + [ + pytest.param({"no_privacy": True, "privacy__epsilon": 1.0}, id="parent-then-child"), + pytest.param({"privacy__epsilon": 1.0, "no_privacy": True}, id="child-then-parent"), + ], +) +def test_parse_overrides_rejects_parent_child_collisions(values: dict[str, object]): + with pytest.raises(ValueError, match=r"Conflicting override paths.*privacy"): + parse_overrides(values) # --------------------------------------------------------------------------- diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py index f92a43df2..e55207344 100644 --- a/tests/sdk/test_config_builder.py +++ b/tests/sdk/test_config_builder.py @@ -13,6 +13,8 @@ DifferentialPrivacyHyperparams, EvaluationParameters, GenerateParameters, + PreflightParameters, + SafeSynthesizerParameters, TimeSeriesParameters, TrainingHyperparams, ) @@ -195,6 +197,15 @@ def test_resolved_config_is_independent_of_mapping_source(): assert builder._nss_config.evaluation.pii_replay_entities == ["email"] +def test_resolve_preserves_preflight_from_existing_config(): + config = SafeSynthesizerParameters(preflight=PreflightParameters(disabled_checks=["gpu.vram"])) + + builder = ConfigBuilder(config).with_data_source(pd.DataFrame({"value": [1]})).resolve() + + assert builder._nss_config is not None + assert builder._nss_config.preflight.disabled_checks == ["gpu.vram"] + + def test_direct_assembly_preserves_classify_model_provider_injection(): builder = ConfigBuilder().with_data_source(pd.DataFrame({"value": [1]})) builder._classify_model_provider = "test-provider" From 4d3aef545e69066486c29e7ac43b95b0c03845b9 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Tue, 30 Jun 2026 21:37:39 +0000 Subject: [PATCH 14/19] fix(config): mark parameter path string override Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/configurator/parameter_paths.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index 1000d8235..bc6cebe58 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -10,6 +10,8 @@ from enum import Enum, auto from typing import TYPE_CHECKING, Annotated, Self, Union, cast, get_args, get_origin +from typing_extensions import override + if TYPE_CHECKING: from .parameters import Parameters @@ -24,6 +26,7 @@ def __post_init__(self) -> None: if not self.parts or any(not part for part in self.parts): raise ValueError("A parameter path cannot contain empty segments.") + @override def __str__(self) -> str: return ".".join(self.parts) From 596c7930183369d4070c1aa15d3ec91bc8891f84 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 1 Jul 2026 20:20:15 +0000 Subject: [PATCH 15/19] refactor(config): clarify patch ownership Signed-off-by: Aaron Gonzales --- docs/user-guide/configuration.md | 2 +- src/nemo_safe_synthesizer/config/generate.py | 42 ++--- .../config/parameters.py | 44 ++--- src/nemo_safe_synthesizer/config/patch.py | 143 ++++++++-------- .../configurator/parameter_paths.py | 153 ++++++++++++++---- .../configurator/parameters.py | 32 +++- .../configurator/pydantic_compat.py | 32 ++++ tests/config/test_parameters.py | 77 ++++++++- tests/config/test_patch.py | 46 +++++- tests/sdk/test_config_builder.py | 39 ++++- 10 files changed, 424 insertions(+), 186 deletions(-) create mode 100644 src/nemo_safe_synthesizer/configurator/pydantic_compat.py diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index c0263997a..0b8742a5d 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -62,7 +62,7 @@ for more detail on combining config files with runtime overrides. - top-level fields such as `generation` or `replace_pii`; - canonical dotted paths such as `generation.num_records`; -- bare leaf names such as `num_records`, when that leaf name is unique in the +- bare nested names such as `num_records`, when that name is unique in the configuration schema; and - legacy structured-generation aliases, retained for compatibility. diff --git a/src/nemo_safe_synthesizer/config/generate.py b/src/nemo_safe_synthesizer/config/generate.py index ff02495fe..47b1e05aa 100644 --- a/src/nemo_safe_synthesizer/config/generate.py +++ b/src/nemo_safe_synthesizer/config/generate.py @@ -4,6 +4,7 @@ from __future__ import annotations import warnings +from collections.abc import Mapping from typing import Annotated, Any, ClassVar, Literal, Self from pydantic import ( @@ -12,6 +13,7 @@ model_validator, ) +from ..configurator.parameter_paths import ParameterSchema from ..configurator.parameters import ( Parameters, ) @@ -270,45 +272,19 @@ class GenerateParameters(Parameters, BaseModel): ), ] = "auto" - _STRUCTURED_GENERATION_LEGACY_FIELDS: ClassVar[dict[str, str]] = { - "use_structured_generation": "enabled", - "structured_generation_backend": "backend", - "structured_generation_schema_method": "schema_method", - "structured_generation_use_single_sequence": "use_single_sequence", + parameter_aliases: ClassVar[Mapping[str, str]] = { + "use_structured_generation": "structured_generation.enabled", + "structured_generation_backend": "structured_generation.backend", + "structured_generation_schema_method": "structured_generation.schema_method", + "structured_generation_use_single_sequence": "structured_generation.use_single_sequence", } @model_validator(mode="before") @classmethod def _migrate_legacy_structured_generation_fields(cls, data: Any) -> Any: - if not isinstance(data, dict): + if not isinstance(data, Mapping): return data - - values = dict(data) - legacy = { - new_name: values.pop(old_name) - for old_name, new_name in cls._STRUCTURED_GENERATION_LEGACY_FIELDS.items() - if old_name in values - } - if not legacy: - return values - - structured_generation = values.get("structured_generation") - match structured_generation: - case StructuredGenerationParameters() as params: - structured_values = params.model_dump() - case BaseModel() as model: - structured_values = model.model_dump() - case dict() as mapping: - structured_values = dict(mapping) - case None: - structured_values = {} - case _: - return values - - # Legacy flat keys are treated as explicit overrides for migration - # paths such as ``from_params(generation={...}, structured_generation_backend=...)``. - values["structured_generation"] = structured_values | legacy - return values + return ParameterSchema.from_model(cls).normalize_aliases(data) @property def use_structured_generation(self) -> bool: diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index f7899163b..6e82afb69 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -11,11 +11,8 @@ from typing_extensions import override from ..configurator.parameter_paths import ( - AmbiguousParameterName, ParameterPath, ParameterSchema, - ResolvedParameterName, - UnknownParameterName, ) from ..configurator.parameters import Parameters from ..errors import ParameterError @@ -40,31 +37,6 @@ logger = get_logger(__name__) -_LEGACY_FLAT_PATHS: dict[str, tuple[str, ...]] = { - "use_structured_generation": ("generation", "structured_generation", "enabled"), - "structured_generation_backend": ("generation", "structured_generation", "backend"), - "structured_generation_schema_method": ("generation", "structured_generation", "schema_method"), - "structured_generation_use_single_sequence": ("generation", "structured_generation", "use_single_sequence"), -} - - -def _resolve_parameter_name(schema: ParameterSchema, name: str) -> ParameterPath: - """Resolve one ``from_params`` name while retaining legacy alias policy.""" - if name not in schema.model_type.model_fields and name in _LEGACY_FLAT_PATHS: - return ParameterPath(_LEGACY_FLAT_PATHS[name]) - - match schema.resolve(name): - case ResolvedParameterName() as resolved: - return resolved.path - case UnknownParameterName() as unknown: - kind = "path" if "." in name else "name" - raise ParameterError(f"Unknown parameter {kind} {unknown.name!r}.") - case AmbiguousParameterName() as ambiguous: - choices = ", ".join(str(path) for path in ambiguous.candidates) - raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") - raise ParameterError(f"Unexpected parameter resolution for {name!r}.") - - class SafeSynthesizerParameters(Parameters): """Main configuration class for the Safe Synthesizer pipeline. @@ -196,7 +168,7 @@ def check_timeseries_group_column(self) -> Self: def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": """Construct parameters from resolved keyword names. - Names may be top-level fields, canonical dotted paths, unique bare leaf + Names may be top-level fields, canonical dotted paths, unique bare names, or supported legacy aliases. Ambiguous bare names raise an error that lists the canonical dotted alternatives. @@ -214,7 +186,7 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": assignments: list[PatchAssignment] = [] resolved_paths: set[ParameterPath] = set() for name, value in kwargs.items(): - path = _resolve_parameter_name(schema, name) + path = schema.require(name) if path in resolved_paths: raise ParameterError(f"Duplicate parameter path {str(path)!r}.") resolved_paths.add(path) @@ -225,7 +197,10 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": @classmethod def from_config_patch(cls, patch: ConfigPatch) -> Self: """Validate a sparse top-level config patch as a full configuration.""" - return CompiledConfigPatch.from_mapping(cls, patch, origin="config patch", precedence=0).apply() + normalized = ParameterSchema.from_model(cls).normalize_aliases(patch) + return CompiledConfigPatch.from_mapping( + cls, normalized, origin="config patch", precedence=0, unknown_fields="ignore" + ).apply() def with_config_patch(self, patch: ConfigPatch) -> Self: """Apply a sparse top-level config patch and revalidate the result. @@ -240,8 +215,12 @@ def with_config_patch(self, patch: ConfigPatch) -> Self: self.model_dump(exclude_unset=True), origin="base config", precedence=0, + unknown_fields="reject", + ) + normalized = ParameterSchema.from_model(model_type).normalize_aliases(patch) + override = CompiledConfigPatch.from_mapping( + model_type, normalized, origin="config patch", precedence=1, unknown_fields="ignore" ) - override = CompiledConfigPatch.from_mapping(model_type, patch, origin="config patch", precedence=1) return base.combine(override).apply() def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSynthesizerParameters": @@ -277,5 +256,6 @@ def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSyn updates, origin="runtime override", precedence=1, + unknown_fields="reject", ) return self.apply_patch(patch) diff --git a/src/nemo_safe_synthesizer/config/patch.py b/src/nemo_safe_synthesizer/config/patch.py index 4a52cf0bc..89756dc1a 100644 --- a/src/nemo_safe_synthesizer/config/patch.py +++ b/src/nemo_safe_synthesizer/config/patch.py @@ -5,18 +5,19 @@ from __future__ import annotations -import types from collections.abc import Iterable, Mapping from copy import deepcopy from dataclasses import dataclass -from typing import Annotated, Generic, TypeVar, Union, cast, get_args, get_origin +from typing import Generic, Literal, TypeAlias, TypeVar, cast from pydantic import BaseModel from ..configurator.parameter_paths import ParameterPath +from ..configurator.pydantic_compat import nested_model_type from ..errors import ParameterError ModelT = TypeVar("ModelT", bound=BaseModel) +UnknownFieldBehavior: TypeAlias = Literal["ignore", "reject"] @dataclass(frozen=True, slots=True) @@ -54,9 +55,18 @@ def from_mapping( *, origin: str, precedence: int, + unknown_fields: UnknownFieldBehavior, ) -> CompiledConfigPatch[ModelT]: + """Compile a mapping with an explicit policy for traversable unknown fields. + + Collections are atomic patch leaves. Pydantic validates their contents + when the patch is applied. + """ _require_model_type(target_model) - assignments = _mapping_assignments(target_model, source, origin=origin, precedence=precedence) + if unknown_fields not in ("ignore", "reject"): + raise ValueError(f"Unsupported unknown-field behavior {unknown_fields!r}.") + compatible_source = _mapping_without_extras(target_model, source) if unknown_fields == "ignore" else source + assignments = _mapping_assignments(target_model, compatible_source, origin=origin, precedence=precedence) return CompiledConfigPatch.from_paths(target_model, assignments) @staticmethod @@ -64,12 +74,10 @@ def from_model( target_model: type[ModelT], source: ModelT, *, origin: str, precedence: int ) -> CompiledConfigPatch[ModelT]: _require_exact_model(target_model, source) - return CompiledConfigPatch.from_mapping( - target_model, - _extract_set_fields(source), - origin=origin, - precedence=precedence, + assignments = _mapping_assignments( + target_model, _extract_set_fields(source), origin=origin, precedence=precedence ) + return CompiledConfigPatch.from_paths(target_model, assignments) def combine(self, other: CompiledConfigPatch[ModelT]) -> CompiledConfigPatch[ModelT]: if self.target_model is not other.target_model: @@ -97,8 +105,8 @@ def apply(self, base: ModelT | None = None) -> ModelT: _merge_model_mapping(values, self.target_model, self.materialize()) return self.target_model.model_validate(values) - def _apply_to_full_model(self, base: ModelT) -> ModelT: - """Apply to current full values while retaining sparse field presence.""" + def apply_to_full_model(self, base: ModelT) -> ModelT: + """Apply to full current values while retaining sparse field presence.""" _require_exact_model(self.target_model, base) patch_values = self.materialize() values = base.model_dump() @@ -118,30 +126,13 @@ def _require_exact_model(model_type: type[ModelT], value: BaseModel) -> None: raise TypeError(f"Patch target model is {model_type.__name__}; received {type(value).__name__}.") -def _unwrap_annotation(annotation: object) -> object: - while get_origin(annotation) is Annotated: - annotation = get_args(annotation)[0] - origin = get_origin(annotation) - if origin not in (types.UnionType, Union): - return annotation - members = tuple(_unwrap_annotation(item) for item in get_args(annotation) if item is not type(None)) - return members[0] if len(members) == 1 else annotation - - -def _nested_model_type(annotation: object) -> type[BaseModel] | None: - annotation = _unwrap_annotation(annotation) - if isinstance(annotation, type) and issubclass(annotation, BaseModel): - return annotation - return None - - def _field_model_at_path(model_type: type[BaseModel], path: ParameterPath) -> type[BaseModel] | None: current = model_type for index, part in enumerate(path.parts): field = current.model_fields.get(part) if field is None: raise ParameterError(f"Unknown configuration path {str(path)!r}.") - nested = _nested_model_type(field.annotation) + nested = nested_model_type(field.annotation, BaseModel) if index == len(path.parts) - 1: return nested if nested is None: @@ -161,18 +152,21 @@ def _mapping_assignments( ) -> tuple[PatchAssignment, ...]: assignments: list[PatchAssignment] = [] for name, value in source.items(): - # Raw mappings retain Pydantic's extra-ignore adapter contract. Canonical - # assignments use from_paths, which validates every resolved path. if name not in model_type.model_fields: - continue + path = ".".join((*prefix, name)) + raise ParameterError(f"Unknown configuration path {path!r}.") path = ParameterPath((*prefix, name)) nested = _field_model_at_path(model_type, ParameterPath((name,))) nested_source = _branch_mapping(nested, value) if nested is None or nested_source is None or not nested_source: - assignments.append(PatchAssignment(path, deepcopy(value), origin, precedence)) + assignments.append(PatchAssignment(path, value, origin, precedence)) continue nested_assignments = _mapping_assignments( - nested, nested_source, origin=origin, precedence=precedence, prefix=path.parts + nested, + nested_source, + origin=origin, + precedence=precedence, + prefix=path.parts, ) if nested_assignments: assignments.extend(nested_assignments) @@ -183,6 +177,23 @@ def _mapping_assignments( return tuple(assignments) +def _mapping_without_extras(model_type: type[BaseModel], source: Mapping[str, object]) -> dict[str, object]: + """Adapt a raw mapping to Pydantic's recursive extra-ignore behavior.""" + adapted: dict[str, object] = {} + for name, value in source.items(): + field = model_type.model_fields.get(name) + if field is None: + continue + nested_model = nested_model_type(field.annotation, BaseModel) + nested_source = _branch_mapping(nested_model, value) + adapted[name] = ( + _mapping_without_extras(nested_model, nested_source) + if nested_model is not None and nested_source is not None + else value + ) + return adapted + + def _branch_mapping(nested_model: type[BaseModel] | None, value: object) -> Mapping[str, object] | None: if nested_model is None: return None @@ -196,7 +207,7 @@ def _branch_mapping(nested_model: type[BaseModel] | None, value: object) -> Mapp def _extract_set_fields(model: BaseModel) -> dict[str, object]: extracted: dict[str, object] = {} for name in type(model).model_fields: - value = model.__dict__[name] + value = _stored_field_value(model, name) if isinstance(value, BaseModel): nested = _extract_set_fields(value) if nested or name in model.model_fields_set: @@ -207,45 +218,39 @@ def _extract_set_fields(model: BaseModel) -> dict[str, object]: return extracted +def _stored_field_value(model: BaseModel, name: str) -> object: + """Read a validated field without triggering Pydantic access warnings.""" + return vars(model)[name] + + def _restore_model_fields_set(result: BaseModel, base: BaseModel, patch: Mapping[str, object]) -> None: """Restore recursive base presence and add fields supplied by ``patch``.""" - object.__setattr__(result, "__pydantic_fields_set__", set(base.model_fields_set)) + _replace_model_fields_set(result, (*base.model_fields_set, *patch)) for name in type(result).model_fields: - result_value = result.__dict__[name] + result_value = _stored_field_value(result, name) if not isinstance(result_value, BaseModel): continue - base_value = base.__dict__[name] + base_value = _stored_field_value(base, name) + nested_patch = _branch_mapping(type(result_value), patch.get(name)) or {} if isinstance(base_value, BaseModel): - _restore_model_fields_set(result_value, base_value, {}) + _restore_model_fields_set(result_value, base_value, nested_patch) else: - _clear_model_fields_set(result_value) - - result.__pydantic_fields_set__.update(patch) - for name, value in patch.items(): - result_value = result.__dict__[name] - if not isinstance(result_value, BaseModel): - continue - nested_patch = _branch_mapping(type(result_value), value) - if nested_patch is not None: - _add_model_fields_set(result_value, nested_patch) + _set_model_fields_from_patch(result_value, nested_patch) -def _clear_model_fields_set(model: BaseModel) -> None: - object.__setattr__(model, "__pydantic_fields_set__", set()) - for value in model.__dict__.values(): - if isinstance(value, BaseModel): - _clear_model_fields_set(value) +def _set_model_fields_from_patch(model: BaseModel, patch: Mapping[str, object]) -> None: + _replace_model_fields_set(model, patch) + for name in type(model).model_fields: + value = _stored_field_value(model, name) + if not isinstance(value, BaseModel): + continue + nested_patch = _branch_mapping(type(value), patch.get(name)) or {} + _set_model_fields_from_patch(value, nested_patch) -def _add_model_fields_set(model: BaseModel, patch: Mapping[str, object]) -> None: - model.__pydantic_fields_set__.update(patch) - for name, value in patch.items(): - model_value = model.__dict__[name] - if not isinstance(model_value, BaseModel): - continue - nested_patch = _branch_mapping(type(model_value), value) - if nested_patch is not None: - _add_model_fields_set(model_value, nested_patch) +def _replace_model_fields_set(model: BaseModel, fields: Iterable[str]) -> None: + model.model_fields_set.clear() + model.model_fields_set.update(fields) def _validate_conflicts(model_type: type[BaseModel], assignments: tuple[PatchAssignment, ...]) -> None: @@ -280,12 +285,12 @@ def _insert_value( ) -> None: name, *tail = parts field = model_type.model_fields[name] - nested_model = _nested_model_type(field.annotation) + nested_model = nested_model_type(field.annotation, BaseModel) if tail: if nested_model is None: raise AssertionError("Validated paths cannot descend through atomic fields.") branch = target.get(name) - nested = _as_object_dict(branch) + nested = cast(dict[str, object], branch) if isinstance(branch, dict) else {} target[name] = nested _insert_value(nested, nested_model, tuple(tail), value) return @@ -296,19 +301,13 @@ def _insert_value( if nested_model is None: raise AssertionError("A branch mapping must have a nested model type.") branch = target.get(name) - nested = _as_object_dict(branch) + nested = cast(dict[str, object], branch) if isinstance(branch, dict) else {} target[name] = nested _merge_model_mapping(nested, nested_model, branch_source) -def _as_object_dict(value: object) -> dict[str, object]: - if isinstance(value, dict): - return cast(dict[str, object], value) - return {} - - def _merge_model_mapping(target: dict[str, object], model_type: type[BaseModel], source: Mapping[str, object]) -> None: for name, value in source.items(): if name not in model_type.model_fields: raise ParameterError(f"Unknown configuration path {name!r} for {model_type.__name__}.") - _insert_value(target, model_type, (name,), deepcopy(value)) + _insert_value(target, model_type, (name,), value) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index bc6cebe58..835a77ef9 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -5,13 +5,17 @@ from __future__ import annotations -import types +from collections.abc import Mapping from dataclasses import dataclass from enum import Enum, auto -from typing import TYPE_CHECKING, Annotated, Self, Union, cast, get_args, get_origin +from typing import TYPE_CHECKING, Self, cast +from pydantic import BaseModel from typing_extensions import override +from ..errors import ParameterError +from .pydantic_compat import nested_model_type + if TYPE_CHECKING: from .parameters import Parameters @@ -45,25 +49,10 @@ def classify_parameter_annotation(annotation: object) -> ParameterFieldKind: return ParameterFieldKind.LEAF -def _unwrap_annotated(annotation: object) -> object: - while get_origin(annotation) is Annotated: - annotation = get_args(annotation)[0] - return annotation - - def _nested_parameters_type(annotation: object) -> type[Parameters] | None: from .parameters import Parameters - annotation = _unwrap_annotated(annotation) - origin = get_origin(annotation) - if origin in (types.UnionType, Union): - members = tuple(_unwrap_annotated(member) for member in get_args(annotation) if member is not type(None)) - if len(members) != 1: - return None - annotation = members[0] - if isinstance(annotation, type) and issubclass(annotation, Parameters): - return annotation - return None + return nested_model_type(annotation, Parameters) @dataclass(frozen=True, slots=True) @@ -74,6 +63,14 @@ class ParameterField: kind: ParameterFieldKind +@dataclass(frozen=True, slots=True) +class ParameterAlias: + """One accepted compatibility name and its canonical parameter path.""" + + name: str + path: ParameterPath + + @dataclass(frozen=True, slots=True) class ResolvedParameterName: """A parameter name resolved to one canonical path.""" @@ -105,6 +102,7 @@ class ParameterSchema: model_type: type[Parameters] fields: tuple[ParameterField, ...] + aliases: tuple[ParameterAlias, ...] @classmethod def from_model(cls, model_type: type[Parameters]) -> Self: @@ -114,9 +112,16 @@ def from_model(cls, model_type: type[Parameters]) -> Self: if not issubclass(model_type, Parameters): raise TypeError(f"Expected a Parameters model type, received {model_type!r}.") fields = tuple(_iter_parameter_fields(model_type)) - return cls(model_type=model_type, fields=fields) - - def resolve(self, name: str) -> ParameterNameResolution: + aliases = tuple(_iter_parameter_aliases(model_type)) + field_paths = {field.path for field in fields} + for alias in aliases: + if alias.path not in field_paths: + raise TypeError( + f"Parameter alias {alias.name!r} on {model_type.__name__} targets unknown path {str(alias.path)!r}." + ) + return cls(model_type=model_type, fields=fields, aliases=aliases) + + def resolve(self, name: str, *, infer_bare_name: bool = True) -> ParameterNameResolution: """Resolve a canonical dotted or bare parameter name.""" if "." in name: try: @@ -125,17 +130,80 @@ def resolve(self, name: str) -> ParameterNameResolution: return UnknownParameterName(name) if any(field.path == requested for field in self.fields): return ResolvedParameterName(requested) - return UnknownParameterName(name) + return _resolution_from_candidates(name, self._alias_candidates(name)) top_level = next((field.path for field in self.fields if field.path.parts == (name,)), None) if top_level is not None: return ResolvedParameterName(top_level) - candidates = tuple(field.path for field in self.fields if field.path.parts[-1] == name) - if not candidates: + aliases = self._alias_candidates(name) + if aliases: + return _resolution_from_candidates(name, aliases) + if not infer_bare_name: return UnknownParameterName(name) - if len(candidates) > 1: - return AmbiguousParameterName(name, candidates) - return ResolvedParameterName(candidates[0]) + candidates = tuple(field.path for field in self.fields if field.path.parts[-1] == name) + return _resolution_from_candidates(name, candidates) + + def require(self, name: str, *, infer_bare_name: bool = True) -> ParameterPath: + """Resolve one name or raise a user-facing configuration error.""" + resolution = self.resolve(name, infer_bare_name=infer_bare_name) + if not infer_bare_name and isinstance(resolution, UnknownParameterName) and "." not in name: + inferred = tuple(field.path for field in self.fields if field.path.parts[-1] == name) + if len(inferred) == 1: + path = inferred[0] + parent = path.parts[0] + raise ParameterError( + f"Nested parameter name {name!r} is not a direct override; " + f"use {str(path)!r} or pass the {parent!r} mapping." + ) + if len(inferred) > 1: + resolution = AmbiguousParameterName(name, inferred) + + match resolution: + case ResolvedParameterName() as resolved: + return resolved.path + case UnknownParameterName() as unknown: + kind = "path" if "." in name else "name" + raise ParameterError(f"Unknown parameter {kind} {unknown.name!r}.") + case AmbiguousParameterName() as ambiguous: + choices = ", ".join(str(path) for path in ambiguous.candidates) + raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") + + def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: + """Translate declared aliases to canonical paths, with aliases taking precedence.""" + values = dict(source) + for name, field_info in self.model_type.model_fields.items(): + nested_type = _nested_parameters_type(field_info.annotation) + value = values.get(name) + if nested_type is not None and isinstance(value, Mapping): + values[name] = ParameterSchema.from_model(nested_type).normalize_aliases( + cast(Mapping[str, object], value) + ) + + for name in tuple(values): + if name in self.model_type.model_fields: + continue + candidates = self._alias_candidates(name) + if not candidates: + continue + resolution = _resolution_from_candidates(name, candidates) + if isinstance(resolution, AmbiguousParameterName): + choices = ", ".join(str(path) for path in resolution.candidates) + raise ParameterError(f"Ambiguous parameter alias {name!r}; use one of: {choices}.") + if isinstance(resolution, ResolvedParameterName): + _set_parameter_value(values, resolution.path, values.pop(name)) + return values + + def _alias_candidates(self, name: str) -> tuple[ParameterPath, ...]: + return tuple(alias.path for alias in self.aliases if alias.name == name) + + +def _resolution_from_candidates(name: str, candidates: tuple[ParameterPath, ...]) -> ParameterNameResolution: + unique = tuple(sorted(set(candidates), key=lambda path: path.parts)) + if not unique: + return UnknownParameterName(name) + if len(unique) > 1: + return AmbiguousParameterName(name, unique) + return ResolvedParameterName(unique[0]) def _iter_parameter_fields(model_type: type[Parameters], prefix: tuple[str, ...] = ()) -> tuple[ParameterField, ...]: @@ -151,6 +219,37 @@ def _iter_parameter_fields(model_type: type[Parameters], prefix: tuple[str, ...] return tuple(fields) +def _iter_parameter_aliases(model_type: type[Parameters], prefix: tuple[str, ...] = ()) -> tuple[ParameterAlias, ...]: + aliases: list[ParameterAlias] = [] + for name, target in model_type.parameter_aliases.items(): + target_path = split_parameter_path(target) + canonical_path = ParameterPath((*prefix, *target_path.parts)) + aliases.append(ParameterAlias(name, canonical_path)) + if prefix: + aliases.append(ParameterAlias(".".join((*prefix, name)), canonical_path)) + + for name, field_info in model_type.model_fields.items(): + nested_type = _nested_parameters_type(field_info.annotation) + if nested_type is not None: + aliases.extend(_iter_parameter_aliases(nested_type, (*prefix, name))) + return tuple(aliases) + + +def _set_parameter_value(target: dict[str, object], path: ParameterPath, value: object) -> None: + current = target + for part in path.parts[:-1]: + branch = current.get(part) + if isinstance(branch, BaseModel): + nested = branch.model_dump(exclude_unset=True) + elif isinstance(branch, Mapping): + nested = dict(branch) + else: + nested = {} + current[part] = nested + current = nested + current[path.parts[-1]] = value + + def split_parameter_path(name: str, separator: str = ".") -> ParameterPath: """Split a parameter name into a canonical path.""" if not separator: diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index d15f98e08..a1d910608 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -21,7 +21,7 @@ from abc import ABCMeta from collections.abc import Generator, Iterator, Mapping from pathlib import Path -from typing import Any, Self, cast, get_args +from typing import Any, ClassVar, Self, cast, get_args import yaml from pydantic import ( @@ -31,11 +31,12 @@ from ..config.base import ( pydantic_model_config, ) -from ..config.patch import CompiledConfigPatch +from ..config.patch import CompiledConfigPatch, PatchAssignment from ..errors import ParameterError from .parameter import ( DataT, ) +from .parameter_paths import ParameterSchema __all__ = ["Parameters"] @@ -52,6 +53,7 @@ class Parameters(BaseModel, metaclass=ABCMeta): """ model_config = pydantic_model_config + parameter_aliases: ClassVar[Mapping[str, str]] = {} def explicit_patch(self) -> CompiledConfigPatch[Self]: """Compile this model's recursively explicit fields as a sparse patch. @@ -70,19 +72,26 @@ def apply_patch(self, patch: CompiledConfigPatch[Self]) -> Self: resolved defaults keep their current values. Patch assignments retain their relative precedence and always follow the base. """ - return patch._apply_to_full_model(self) + return patch.apply_to_full_model(self) @classmethod def from_config_source(cls, source: Self | Mapping[str, object] | None = None, **kwargs: object) -> Self: """Normalize one sparse config source plus higher-precedence keyword values. ``source`` may be ``None``, an instance of exactly ``cls``, or a raw - mapping. Unknown mapping keys retain Pydantic's extra-ignore behavior. - A different Pydantic model type is rejected rather than adapted. + mapping. Declared compatibility aliases are normalized for raw mappings + and keyword overrides. Unknown mapping keys retain Pydantic's + extra-ignore behavior. Keyword overrides accept top-level fields and + canonical dotted paths, but reject inferred bare nested names with an + actionable path suggestion. A different Pydantic model type is rejected + rather than adapted. """ + schema = ParameterSchema.from_model(cls) match source: case None: - source_patch = CompiledConfigPatch.from_mapping(cls, {}, origin="empty config", precedence=0) + source_patch = CompiledConfigPatch.from_mapping( + cls, {}, origin="empty config", precedence=0, unknown_fields="reject" + ) case BaseModel() as model: if type(model) is not cls: raise TypeError(f"Expected {cls.__name__}, got {type(model).__name__}") @@ -95,14 +104,21 @@ def from_config_source(cls, source: Self | Mapping[str, object] | None = None, * case Mapping() as mapping: source_patch = CompiledConfigPatch.from_mapping( cls, - cast(Mapping[str, object], mapping), + schema.normalize_aliases(cast(Mapping[str, object], mapping)), origin="mapping config", precedence=0, + unknown_fields="ignore", ) case _: raise TypeError(f"Unsupported config type: {type(source)}") - overrides = CompiledConfigPatch.from_mapping(cls, kwargs, origin="keyword override", precedence=1) + overrides = CompiledConfigPatch.from_paths( + cls, + ( + PatchAssignment(schema.require(name, infer_bare_name=False), value, f"keyword override {name!r}", 1) + for name, value in kwargs.items() + ), + ) return source_patch.combine(overrides).apply() def _isparams(self): diff --git a/src/nemo_safe_synthesizer/configurator/pydantic_compat.py b/src/nemo_safe_synthesizer/configurator/pydantic_compat.py new file mode 100644 index 000000000..31b1e1bd4 --- /dev/null +++ b/src/nemo_safe_synthesizer/configurator/pydantic_compat.py @@ -0,0 +1,32 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Low-level Pydantic annotation compatibility helpers.""" + +from __future__ import annotations + +import types +from typing import Annotated, TypeVar, Union, get_args, get_origin + +from pydantic import BaseModel + +ModelT = TypeVar("ModelT", bound=BaseModel) + + +def unwrap_optional_annotation(annotation: object) -> object: + """Unwrap ``Annotated`` and a union with one non-``None`` member.""" + while get_origin(annotation) is Annotated: + annotation = get_args(annotation)[0] + origin = get_origin(annotation) + if origin not in (types.UnionType, Union): + return annotation + members = tuple(unwrap_optional_annotation(item) for item in get_args(annotation) if item is not type(None)) + return members[0] if len(members) == 1 else annotation + + +def nested_model_type(annotation: object, expected_base: type[ModelT]) -> type[ModelT] | None: + """Return the nested model type when an annotation has one compatible model.""" + annotation = unwrap_optional_annotation(annotation) + if isinstance(annotation, type) and issubclass(annotation, expected_base): + return annotation + return None diff --git a/tests/config/test_parameters.py b/tests/config/test_parameters.py index 522c5cba1..1c3336777 100644 --- a/tests/config/test_parameters.py +++ b/tests/config/test_parameters.py @@ -10,12 +10,14 @@ import pytest from pydantic import Field, ValidationError, model_validator +from nemo_safe_synthesizer.config.generate import GenerateParameters, StructuredGenerationParameters from nemo_safe_synthesizer.config.parameters import SafeSynthesizerParameters from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig, StepDefinition from nemo_safe_synthesizer.config.training import QuantizationScheme from nemo_safe_synthesizer.configurator.parameter_paths import ( AmbiguousParameterName, ParameterFieldKind, + ParameterPath, ParameterSchema, ResolvedParameterName, UnknownParameterName, @@ -202,13 +204,6 @@ def test_from_params_rejects_unknown_flat_parameter(): SafeSynthesizerParameters.from_params(not_a_parameter=True) -def test_from_params_rejects_unexpected_parameter_resolution(monkeypatch): - monkeypatch.setattr(ParameterSchema, "resolve", lambda _schema, _name: object()) - - with pytest.raises(ParameterError, match="Unexpected parameter resolution for 'num_records'"): - SafeSynthesizerParameters.from_params(num_records=10) - - def test_parameter_schema_indexes_optional_branches_without_an_instance(): with patch.object(SafeSynthesizerParameters, "__init__", side_effect=AssertionError("model instantiated")): schema = ParameterSchema.from_model(SafeSynthesizerParameters) @@ -304,6 +299,10 @@ class _DuplicateLeafParameters(Parameters): right: _RightParameters = Field(default_factory=_RightParameters) +class _NestedParameters(Parameters): + child: _LeftParameters = Field(default_factory=_LeftParameters) + + class _PresenceParameters(Parameters): left: _LeftParameters = Field(default_factory=_LeftParameters) right: _RightParameters = Field(default_factory=_RightParameters) @@ -374,6 +373,70 @@ def test_from_config_source_kwargs_override_source_and_preserve_nested_siblings( assert result.right.value == 6 +def test_from_config_source_kwargs_resolve_dotted_nested_parameter_name(): + result = _NestedParameters.from_config_source( + **{"child.value": 9} # ty: ignore[invalid-argument-type] -- dotted names require dynamic keywords + ) + + assert result.child.value == 9 + assert result.model_dump(exclude_unset=True) == {"child": {"value": 9}} + + +def test_from_config_source_rejects_inferred_bare_nested_parameter_name(): + with pytest.raises(ParameterError, match=r"Nested parameter name 'value'.*child\.value.*child"): + _NestedParameters.from_config_source(value=9) + + +def test_from_config_source_rejects_unknown_keyword_override(): + with pytest.raises(ParameterError, match="Unknown parameter name 'unknown'"): + _NestedParameters.from_config_source(unknown=9) + + +def test_from_config_source_rejects_ambiguous_keyword_override(): + with pytest.raises(ParameterError, match=r"Ambiguous parameter name 'value'.*left\.value.*right\.value"): + _DuplicateLeafParameters.from_config_source(value=9) + + +@pytest.mark.parametrize( + ("model_type", "name", "expected"), + [ + pytest.param( + GenerateParameters, + "use_structured_generation", + ParameterPath(("structured_generation", "enabled")), + id="section", + ), + pytest.param( + SafeSynthesizerParameters, + "use_structured_generation", + ParameterPath(("generation", "structured_generation", "enabled")), + id="nested-bare", + ), + pytest.param( + SafeSynthesizerParameters, + "generation.use_structured_generation", + ParameterPath(("generation", "structured_generation", "enabled")), + id="nested-dotted", + ), + ], +) +def test_parameter_schema_resolves_model_declared_aliases( + model_type: type[Parameters], name: str, expected: ParameterPath +): + assert ParameterSchema.from_model(model_type).resolve(name) == ResolvedParameterName(expected) + + +def test_alias_normalization_preserves_sparse_typed_canonical_branch(): + result = GenerateParameters.from_config_source( + { + "structured_generation": StructuredGenerationParameters(backend="guidance"), + "use_structured_generation": True, + } + ) + + assert result.model_dump(exclude_unset=True) == {"structured_generation": {"enabled": True, "backend": "guidance"}} + + def test_from_config_source_copies_mapping_and_returned_mutable_state(): nested = {"items": [1]} source = {"payload": nested} diff --git a/tests/config/test_patch.py b/tests/config/test_patch.py index 93bf3f25d..fa988f5d4 100644 --- a/tests/config/test_patch.py +++ b/tests/config/test_patch.py @@ -46,7 +46,9 @@ def _paths(*assignments: PatchAssignment) -> CompiledConfigPatch[_PatchTarget]: def test_mapping_leaf_with_nested_dictionaries_is_atomic_and_isolated() -> None: fallback = {"fallback": "name"} source = {"vars": {"template": {"given": ["first", fallback]}}} - patch = CompiledConfigPatch.from_mapping(StepDefinition, source, origin="mapping", precedence=0) + patch = CompiledConfigPatch.from_mapping( + StepDefinition, source, origin="mapping", precedence=0, unknown_fields="reject" + ) fallback["fallback"] = "changed" first = patch.apply() @@ -61,7 +63,11 @@ def test_nested_nss_model_branch_patch_preserves_pii_global_siblings() -> None: base = PiiReplacerConfig.get_default_config() original_entities = deepcopy(base.globals.classify.entities) patch = CompiledConfigPatch.from_mapping( - PiiReplacerConfig, {"globals": {"seed": 17}}, origin="override", precedence=1 + PiiReplacerConfig, + {"globals": {"seed": 17}}, + origin="override", + precedence=1, + unknown_fields="reject", ) result = patch.apply(base) @@ -149,7 +155,9 @@ def test_higher_precedence_parent_seed_wins_only_its_overlapping_children() -> N def test_absence_explicit_none_and_explicit_default_remain_distinct() -> None: - absent = CompiledConfigPatch.from_mapping(_PatchTarget, {}, origin="empty", precedence=0).apply() + absent = CompiledConfigPatch.from_mapping( + _PatchTarget, {}, origin="empty", precedence=0, unknown_fields="reject" + ).apply() explicit_none = _paths(_assignment("child", None)).apply() explicit_default = _paths(_assignment("child.count", 3)).apply() @@ -166,6 +174,7 @@ def test_mapping_constructor_ignores_unknown_keys_at_each_model_level() -> None: {"unknown": True, "child": {"unknown": True}}, origin="mapping", precedence=0, + unknown_fields="ignore", ) result = patch.apply() @@ -173,6 +182,34 @@ def test_mapping_constructor_ignores_unknown_keys_at_each_model_level() -> None: assert result.model_dump(exclude_unset=True) == {"child": {}} +@pytest.mark.parametrize( + ("source", "path"), + [ + pytest.param({"unknown": True}, "unknown", id="top-level"), + pytest.param({"child": {"unknown": True}}, "child.unknown", id="nested"), + ], +) +def test_mapping_constructor_can_reject_unknown_keys(source: dict[str, object], path: str) -> None: + with pytest.raises(ParameterError, match=path): + CompiledConfigPatch.from_mapping(_PatchTarget, source, origin="mapping", precedence=0, unknown_fields="reject") + + +def test_unknown_field_rejection_leaves_model_collections_to_pydantic() -> None: + step = PiiReplacerConfig.get_default_config().steps[0].model_dump() + step["unknown"] = True + patch = CompiledConfigPatch.from_mapping( + PiiReplacerConfig, + {"steps": [step]}, + origin="mapping", + precedence=0, + unknown_fields="reject", + ) + + result = patch.apply() + + assert "unknown" not in result.steps[0].model_dump() + + def test_path_constructor_remains_strict_for_unknown_canonical_path() -> None: with pytest.raises(ParameterError, match=r"Unknown configuration path 'unknown'"): _paths(_assignment("unknown", True)) @@ -212,7 +249,7 @@ def test_top_level_validator_runs_at_application_boundary() -> None: def test_wrong_target_model_is_rejected_for_combine_and_apply() -> None: patch = _paths(_assignment("child.count", 6)) - other = CompiledConfigPatch.from_mapping(_OtherTarget, {}, origin="other", precedence=0) + other = CompiledConfigPatch.from_mapping(_OtherTarget, {}, origin="other", precedence=0, unknown_fields="reject") with pytest.raises(TypeError, match="target model"): patch.combine(other) # ty: ignore[invalid-argument-type] -- runtime rejection is the contract @@ -226,6 +263,7 @@ def test_patch_schema_does_not_widen_public_pii_name_resolution() -> None: {"replace_pii": {"globals": {"seed": 3}}}, origin="config", precedence=0, + unknown_fields="reject", ) assert isinstance( diff --git a/tests/sdk/test_config_builder.py b/tests/sdk/test_config_builder.py index e55207344..10cf470ee 100644 --- a/tests/sdk/test_config_builder.py +++ b/tests/sdk/test_config_builder.py @@ -20,6 +20,7 @@ ) from nemo_safe_synthesizer.config.replace_pii import PiiReplacerConfig from nemo_safe_synthesizer.configurator.parameters import Parameters +from nemo_safe_synthesizer.errors import ParameterError from nemo_safe_synthesizer.sdk.config_builder import ConfigBuilder @@ -99,14 +100,48 @@ def test_with_generate_preserves_sparse_typed_config_fields(): builder = ConfigBuilder().with_generate(config=GenerateParameters(num_records=10)) assert builder._generation_config is not None - assert builder._generation_config.__pydantic_fields_set__ == {"num_records"} + assert builder._generation_config.model_fields_set == {"num_records"} def test_with_generate_marks_typed_config_kwargs_as_explicit_fields(): builder = ConfigBuilder().with_generate(config=GenerateParameters(num_records=10), patience=7) assert builder._generation_config is not None - assert builder._generation_config.__pydantic_fields_set__ == {"num_records", "patience"} + assert builder._generation_config.model_fields_set == {"num_records", "patience"} + + +def test_with_generate_accepts_legacy_alias_keyword(): + builder = ConfigBuilder().with_generate(use_structured_generation=True) + + assert builder._generation_config.structured_generation.enabled is True + assert builder._generation_config.model_dump(exclude_unset=True) == {"structured_generation": {"enabled": True}} + + +def test_with_generate_accepts_legacy_alias_in_raw_mapping(): + builder = ConfigBuilder().with_generate(config={"use_structured_generation": True}) + + assert builder._generation_config.structured_generation.enabled is True + + +def test_with_generate_legacy_mapping_alias_overrides_canonical_value(): + builder = ConfigBuilder().with_generate( + config={ + "structured_generation": {"enabled": False}, + "use_structured_generation": True, + } + ) + + assert builder._generation_config.structured_generation.enabled is True + + +def test_with_generate_rejects_duplicate_alias_keyword_path(): + with pytest.raises(ParameterError, match=r"Duplicate parameter path 'structured_generation\.enabled'"): + ConfigBuilder().with_generate( + **{ + "structured_generation.enabled": False, + "use_structured_generation": True, + } # ty: ignore[invalid-argument-type] -- dotted names require dynamic keywords + ) def test_with_generate_rejects_wrong_typed_config_object(): From 0cd8b6a7cf3e70390325f4fdd60f26a6180e8a0c Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Wed, 1 Jul 2026 20:48:09 +0000 Subject: [PATCH 16/19] fix(config): make parameter resolution exhaustive Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/configurator/parameter_paths.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index 835a77ef9..f32e02386 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -167,6 +167,7 @@ def require(self, name: str, *, infer_bare_name: bool = True) -> ParameterPath: case AmbiguousParameterName() as ambiguous: choices = ", ".join(str(path) for path in ambiguous.candidates) raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") + raise ParameterError(f"Unexpected parameter resolution for {name!r}.") def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: """Translate declared aliases to canonical paths, with aliases taking precedence.""" From e250c5e404957a6f569b5ec8a6a9d5963afff23f Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Thu, 2 Jul 2026 19:48:46 +0000 Subject: [PATCH 17/19] refactor(config): centralize parameter-path separator and dedupe resolution Introduce PARAMETER_PATH_SEPARATOR and format_parameter_path as the single source of truth for dotted parameter-path joins/splits, replacing scattered "." literals across patch.py and the configurator. Also extract _fields_ending_with and _ambiguous_error to remove duplicated bare-name lookup and ambiguity-error formatting. Signed-off-by: Aaron Gonzales --- src/nemo_safe_synthesizer/config/patch.py | 6 +-- .../configurator/parameter_paths.py | 46 +++++++++++++------ .../configurator/parameters.py | 14 +++--- 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/patch.py b/src/nemo_safe_synthesizer/config/patch.py index 89756dc1a..d01176eec 100644 --- a/src/nemo_safe_synthesizer/config/patch.py +++ b/src/nemo_safe_synthesizer/config/patch.py @@ -12,7 +12,7 @@ from pydantic import BaseModel -from ..configurator.parameter_paths import ParameterPath +from ..configurator.parameter_paths import ParameterPath, format_parameter_path from ..configurator.pydantic_compat import nested_model_type from ..errors import ParameterError @@ -136,7 +136,7 @@ def _field_model_at_path(model_type: type[BaseModel], path: ParameterPath) -> ty if index == len(path.parts) - 1: return nested if nested is None: - prefix = ".".join(path.parts[: index + 1]) + prefix = format_parameter_path(path.parts[: index + 1]) raise ParameterError(f"Configuration path {str(path)!r} descends through atomic field {prefix!r}.") current = nested raise AssertionError("ParameterPath guarantees at least one path segment.") @@ -153,7 +153,7 @@ def _mapping_assignments( assignments: list[PatchAssignment] = [] for name, value in source.items(): if name not in model_type.model_fields: - path = ".".join((*prefix, name)) + path = format_parameter_path((*prefix, name)) raise ParameterError(f"Unknown configuration path {path!r}.") path = ParameterPath((*prefix, name)) nested = _field_model_at_path(model_type, ParameterPath((name,))) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index f32e02386..b4aa6d7f3 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -5,7 +5,7 @@ from __future__ import annotations -from collections.abc import Mapping +from collections.abc import Iterable, Mapping from dataclasses import dataclass from enum import Enum, auto from typing import TYPE_CHECKING, Self, cast @@ -19,6 +19,13 @@ if TYPE_CHECKING: from .parameters import Parameters +PARAMETER_PATH_SEPARATOR = "." + + +def format_parameter_path(parts: Iterable[str]) -> str: + """Join path segments into the canonical dotted string.""" + return PARAMETER_PATH_SEPARATOR.join(parts) + @dataclass(frozen=True, slots=True) class ParameterPath: @@ -32,7 +39,7 @@ def __post_init__(self) -> None: @override def __str__(self) -> str: - return ".".join(self.parts) + return format_parameter_path(self.parts) class ParameterFieldKind(Enum): @@ -123,7 +130,7 @@ def from_model(cls, model_type: type[Parameters]) -> Self: def resolve(self, name: str, *, infer_bare_name: bool = True) -> ParameterNameResolution: """Resolve a canonical dotted or bare parameter name.""" - if "." in name: + if PARAMETER_PATH_SEPARATOR in name: try: requested = split_parameter_path(name) except ValueError: @@ -140,14 +147,17 @@ def resolve(self, name: str, *, infer_bare_name: bool = True) -> ParameterNameRe return _resolution_from_candidates(name, aliases) if not infer_bare_name: return UnknownParameterName(name) - candidates = tuple(field.path for field in self.fields if field.path.parts[-1] == name) - return _resolution_from_candidates(name, candidates) + return _resolution_from_candidates(name, self._fields_ending_with(name)) def require(self, name: str, *, infer_bare_name: bool = True) -> ParameterPath: """Resolve one name or raise a user-facing configuration error.""" resolution = self.resolve(name, infer_bare_name=infer_bare_name) - if not infer_bare_name and isinstance(resolution, UnknownParameterName) and "." not in name: - inferred = tuple(field.path for field in self.fields if field.path.parts[-1] == name) + if ( + not infer_bare_name + and isinstance(resolution, UnknownParameterName) + and PARAMETER_PATH_SEPARATOR not in name + ): + inferred = self._fields_ending_with(name) if len(inferred) == 1: path = inferred[0] parent = path.parts[0] @@ -162,11 +172,10 @@ def require(self, name: str, *, infer_bare_name: bool = True) -> ParameterPath: case ResolvedParameterName() as resolved: return resolved.path case UnknownParameterName() as unknown: - kind = "path" if "." in name else "name" + kind = "path" if PARAMETER_PATH_SEPARATOR in name else "name" raise ParameterError(f"Unknown parameter {kind} {unknown.name!r}.") case AmbiguousParameterName() as ambiguous: - choices = ", ".join(str(path) for path in ambiguous.candidates) - raise ParameterError(f"Ambiguous parameter name {ambiguous.name!r}; use one of: {choices}.") + raise _ambiguous_error("name", ambiguous.name, ambiguous.candidates) raise ParameterError(f"Unexpected parameter resolution for {name!r}.") def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: @@ -188,8 +197,7 @@ def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: continue resolution = _resolution_from_candidates(name, candidates) if isinstance(resolution, AmbiguousParameterName): - choices = ", ".join(str(path) for path in resolution.candidates) - raise ParameterError(f"Ambiguous parameter alias {name!r}; use one of: {choices}.") + raise _ambiguous_error("alias", name, resolution.candidates) if isinstance(resolution, ResolvedParameterName): _set_parameter_value(values, resolution.path, values.pop(name)) return values @@ -197,6 +205,14 @@ def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: def _alias_candidates(self, name: str) -> tuple[ParameterPath, ...]: return tuple(alias.path for alias in self.aliases if alias.name == name) + def _fields_ending_with(self, name: str) -> tuple[ParameterPath, ...]: + return tuple(field.path for field in self.fields if field.path.parts[-1] == name) + + +def _ambiguous_error(kind: str, name: str, candidates: tuple[ParameterPath, ...]) -> ParameterError: + choices = ", ".join(str(path) for path in candidates) + return ParameterError(f"Ambiguous parameter {kind} {name!r}; use one of: {choices}.") + def _resolution_from_candidates(name: str, candidates: tuple[ParameterPath, ...]) -> ParameterNameResolution: unique = tuple(sorted(set(candidates), key=lambda path: path.parts)) @@ -227,7 +243,7 @@ def _iter_parameter_aliases(model_type: type[Parameters], prefix: tuple[str, ... canonical_path = ParameterPath((*prefix, *target_path.parts)) aliases.append(ParameterAlias(name, canonical_path)) if prefix: - aliases.append(ParameterAlias(".".join((*prefix, name)), canonical_path)) + aliases.append(ParameterAlias(format_parameter_path((*prefix, name)), canonical_path)) for name, field_info in model_type.model_fields.items(): nested_type = _nested_parameters_type(field_info.annotation) @@ -251,7 +267,7 @@ def _set_parameter_value(target: dict[str, object], path: ParameterPath, value: current[path.parts[-1]] = value -def split_parameter_path(name: str, separator: str = ".") -> ParameterPath: +def split_parameter_path(name: str, separator: str = PARAMETER_PATH_SEPARATOR) -> ParameterPath: """Split a parameter name into a canonical path.""" if not separator: raise ValueError("A parameter path separator cannot be empty.") @@ -270,7 +286,7 @@ def insert_parameter_value(target: dict[str, object], path: ParameterPath, value current = cast(dict[str, object], value_at_part) continue if part in current: - prefix = ".".join(path.parts[: index + 1]) + prefix = format_parameter_path(path.parts[: index + 1]) raise ValueError(f"Conflicting override paths for {str(path)!r}: {prefix!r} already has a parent value.") nested: dict[str, object] = {} current[part] = nested diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index a1d910608..019a643c8 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -36,7 +36,7 @@ from .parameter import ( DataT, ) -from .parameter_paths import ParameterSchema +from .parameter_paths import PARAMETER_PATH_SEPARATOR, ParameterSchema, format_parameter_path __all__ = ["Parameters"] @@ -220,15 +220,15 @@ def get(self, name: str, default: Any = None) -> DataT | Any | None: Returns: The parameter value or sub-group if found, otherwise ``default``. """ - if "." in name: - value = self._get_field_path(tuple(name.split("."))) + if PARAMETER_PATH_SEPARATOR in name: + value = self._get_field_path(tuple(name.split(PARAMETER_PATH_SEPARATOR))) return default if value is _MISSING else value matches = [(path, value) for path, value in self._iter_field_paths() if path[-1] == name] if not matches: return default if len(matches) > 1: - candidates = ", ".join(".".join(path) for path, _ in matches) + candidates = ", ".join(format_parameter_path(path) for path, _ in matches) raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") return matches[0][1] @@ -245,11 +245,11 @@ def has(self, name: str) -> bool: Returns: ``True`` if the parameter or sub-group exists. """ - if "." in name: - return self._get_field_path(tuple(name.split("."))) is not _MISSING + if PARAMETER_PATH_SEPARATOR in name: + return self._get_field_path(tuple(name.split(PARAMETER_PATH_SEPARATOR))) is not _MISSING matches = [path for path, _ in self._iter_field_paths() if path[-1] == name] if len(matches) > 1: - candidates = ", ".join(".".join(path) for path in matches) + candidates = ", ".join(format_parameter_path(path) for path in matches) raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") return bool(matches) From b4e0d80ac2d81e1787da12d65399c371746917e8 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Thu, 2 Jul 2026 21:33:28 +0000 Subject: [PATCH 18/19] refactor(config): unify parameter-tree walk and use match for dispatch Extract _walk_parameter_models so _iter_parameter_fields and _iter_parameter_aliases share one pre-order descent. Replace the isinstance chains in _set_parameter_value and normalize_aliases with match statements, mirroring the existing resolution dispatch in require(). Signed-off-by: Aaron Gonzales --- .../configurator/parameter_paths.py | 72 ++++++++++--------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index b4aa6d7f3..04646eb22 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -5,7 +5,7 @@ from __future__ import annotations -from collections.abc import Iterable, Mapping +from collections.abc import Iterable, Iterator, Mapping from dataclasses import dataclass from enum import Enum, auto from typing import TYPE_CHECKING, Self, cast @@ -195,11 +195,13 @@ def normalize_aliases(self, source: Mapping[str, object]) -> dict[str, object]: candidates = self._alias_candidates(name) if not candidates: continue - resolution = _resolution_from_candidates(name, candidates) - if isinstance(resolution, AmbiguousParameterName): - raise _ambiguous_error("alias", name, resolution.candidates) - if isinstance(resolution, ResolvedParameterName): - _set_parameter_value(values, resolution.path, values.pop(name)) + match _resolution_from_candidates(name, candidates): + case AmbiguousParameterName() as ambiguous: + raise _ambiguous_error("alias", name, ambiguous.candidates) + case ResolvedParameterName() as resolved: + _set_parameter_value(values, resolved.path, values.pop(name)) + case UnknownParameterName(): + pass return values def _alias_candidates(self, name: str) -> tuple[ParameterPath, ...]: @@ -223,45 +225,47 @@ def _resolution_from_candidates(name: str, candidates: tuple[ParameterPath, ...] return ResolvedParameterName(unique[0]) -def _iter_parameter_fields(model_type: type[Parameters], prefix: tuple[str, ...] = ()) -> tuple[ParameterField, ...]: - fields: list[ParameterField] = [] +def _walk_parameter_models( + model_type: type[Parameters], prefix: tuple[str, ...] = () +) -> Iterator[tuple[type[Parameters], tuple[str, ...]]]: + """Yield every ``Parameters`` model in the tree with its path prefix, pre-order.""" + yield model_type, prefix for name, field_info in model_type.model_fields.items(): - path = ParameterPath((*prefix, name)) - kind = classify_parameter_annotation(field_info.annotation) - fields.append(ParameterField(path, kind)) - if kind is ParameterFieldKind.BRANCH: - nested_type = _nested_parameters_type(field_info.annotation) - if nested_type is not None: - fields.extend(_iter_parameter_fields(nested_type, path.parts)) + nested_type = _nested_parameters_type(field_info.annotation) + if nested_type is not None: + yield from _walk_parameter_models(nested_type, (*prefix, name)) + + +def _iter_parameter_fields(model_type: type[Parameters]) -> tuple[ParameterField, ...]: + fields: list[ParameterField] = [] + for model, prefix in _walk_parameter_models(model_type): + for name, field_info in model.model_fields.items(): + path = ParameterPath((*prefix, name)) + fields.append(ParameterField(path, classify_parameter_annotation(field_info.annotation))) return tuple(fields) -def _iter_parameter_aliases(model_type: type[Parameters], prefix: tuple[str, ...] = ()) -> tuple[ParameterAlias, ...]: +def _iter_parameter_aliases(model_type: type[Parameters]) -> tuple[ParameterAlias, ...]: aliases: list[ParameterAlias] = [] - for name, target in model_type.parameter_aliases.items(): - target_path = split_parameter_path(target) - canonical_path = ParameterPath((*prefix, *target_path.parts)) - aliases.append(ParameterAlias(name, canonical_path)) - if prefix: - aliases.append(ParameterAlias(format_parameter_path((*prefix, name)), canonical_path)) - - for name, field_info in model_type.model_fields.items(): - nested_type = _nested_parameters_type(field_info.annotation) - if nested_type is not None: - aliases.extend(_iter_parameter_aliases(nested_type, (*prefix, name))) + for model, prefix in _walk_parameter_models(model_type): + for name, target in model.parameter_aliases.items(): + canonical_path = ParameterPath((*prefix, *split_parameter_path(target).parts)) + aliases.append(ParameterAlias(name, canonical_path)) + if prefix: + aliases.append(ParameterAlias(format_parameter_path((*prefix, name)), canonical_path)) return tuple(aliases) def _set_parameter_value(target: dict[str, object], path: ParameterPath, value: object) -> None: current = target for part in path.parts[:-1]: - branch = current.get(part) - if isinstance(branch, BaseModel): - nested = branch.model_dump(exclude_unset=True) - elif isinstance(branch, Mapping): - nested = dict(branch) - else: - nested = {} + match current.get(part): + case BaseModel() as branch: + nested = branch.model_dump(exclude_unset=True) + case Mapping() as branch: + nested = dict(branch) + case _: + nested = {} current[part] = nested current = nested current[path.parts[-1]] = value From c427ecc2148be1671a2f95a8d2c8b5f697fc8624 Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Thu, 2 Jul 2026 22:58:04 +0000 Subject: [PATCH 19/19] refactor(config): dedupe patch helpers and tidy control flow Behavior-preserving cleanup of the config/configurator patch layer: - Extract _ensure_branch and _matching_field_paths helpers to remove duplicated logic in _insert_value and get()/has(). - Centralize the CLI negation prefix into _NEGATION_PREFIX and strip it via removeprefix at the consumer. - Collapse assign-then-guard blocks with the walrus operator. - Unpack (path, value) in get() instead of manual [0][1] indexing. Signed-off-by: Aaron Gonzales --- .../config/parameters.py | 16 +++++----- src/nemo_safe_synthesizer/config/patch.py | 24 +++++++------- .../configurator/parameter_paths.py | 6 ++-- .../configurator/parameters.py | 32 +++++++++++-------- .../configurator/pydantic_click_options.py | 11 +++++-- 5 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/nemo_safe_synthesizer/config/parameters.py b/src/nemo_safe_synthesizer/config/parameters.py index 6e82afb69..d3fe890ea 100644 --- a/src/nemo_safe_synthesizer/config/parameters.py +++ b/src/nemo_safe_synthesizer/config/parameters.py @@ -186,8 +186,7 @@ def from_params(cls, **kwargs: object) -> "SafeSynthesizerParameters": assignments: list[PatchAssignment] = [] resolved_paths: set[ParameterPath] = set() for name, value in kwargs.items(): - path = schema.require(name) - if path in resolved_paths: + if (path := schema.require(name)) in resolved_paths: raise ParameterError(f"Duplicate parameter path {str(path)!r}.") resolved_paths.add(path) assignments.append(PatchAssignment(path, value, f"parameter {name!r}", 0)) @@ -243,12 +242,13 @@ def with_runtime_overrides(self, runtime: SafeSynthesizerParameters) -> "SafeSyn not affect the other. """ updates: dict[str, object] = {} - generation = runtime.generation.explicit_patch().materialize() - if generation or "generation" in runtime.model_fields_set: - updates["generation"] = generation - evaluation = runtime.evaluation.explicit_patch().materialize() - if evaluation or "evaluation" in runtime.model_fields_set: - updates["evaluation"] = evaluation + + def _add_section(name: str, section: Parameters) -> None: + if (materialized := section.explicit_patch().materialize()) or name in runtime.model_fields_set: + updates[name] = materialized + + _add_section("generation", runtime.generation) + _add_section("evaluation", runtime.evaluation) if "emit_telemetry" in runtime.model_fields_set: updates["emit_telemetry"] = runtime.emit_telemetry patch = CompiledConfigPatch.from_mapping( diff --git a/src/nemo_safe_synthesizer/config/patch.py b/src/nemo_safe_synthesizer/config/patch.py index d01176eec..85f2dc1e6 100644 --- a/src/nemo_safe_synthesizer/config/patch.py +++ b/src/nemo_safe_synthesizer/config/patch.py @@ -129,8 +129,7 @@ def _require_exact_model(model_type: type[ModelT], value: BaseModel) -> None: def _field_model_at_path(model_type: type[BaseModel], path: ParameterPath) -> type[BaseModel] | None: current = model_type for index, part in enumerate(path.parts): - field = current.model_fields.get(part) - if field is None: + if (field := current.model_fields.get(part)) is None: raise ParameterError(f"Unknown configuration path {str(path)!r}.") nested = nested_model_type(field.annotation, BaseModel) if index == len(path.parts) - 1: @@ -181,8 +180,7 @@ def _mapping_without_extras(model_type: type[BaseModel], source: Mapping[str, ob """Adapt a raw mapping to Pydantic's recursive extra-ignore behavior.""" adapted: dict[str, object] = {} for name, value in source.items(): - field = model_type.model_fields.get(name) - if field is None: + if (field := model_type.model_fields.get(name)) is None: continue nested_model = nested_model_type(field.annotation, BaseModel) nested_source = _branch_mapping(nested_model, value) @@ -280,6 +278,14 @@ def _ancestor_pair(left: PatchAssignment, right: PatchAssignment) -> tuple[Patch return None, right +def _ensure_branch(target: dict[str, object], name: str) -> dict[str, object]: + """Return ``target[name]`` as a nested dict, replacing any non-dict value.""" + branch = target.get(name) + nested = cast(dict[str, object], branch) if isinstance(branch, dict) else {} + target[name] = nested + return nested + + def _insert_value( target: dict[str, object], model_type: type[BaseModel], parts: tuple[str, ...], value: object ) -> None: @@ -289,10 +295,7 @@ def _insert_value( if tail: if nested_model is None: raise AssertionError("Validated paths cannot descend through atomic fields.") - branch = target.get(name) - nested = cast(dict[str, object], branch) if isinstance(branch, dict) else {} - target[name] = nested - _insert_value(nested, nested_model, tuple(tail), value) + _insert_value(_ensure_branch(target, name), nested_model, tuple(tail), value) return branch_source = _branch_mapping(nested_model, value) if branch_source is None: @@ -300,10 +303,7 @@ def _insert_value( return if nested_model is None: raise AssertionError("A branch mapping must have a nested model type.") - branch = target.get(name) - nested = cast(dict[str, object], branch) if isinstance(branch, dict) else {} - target[name] = nested - _merge_model_mapping(nested, nested_model, branch_source) + _merge_model_mapping(_ensure_branch(target, name), nested_model, branch_source) def _merge_model_mapping(target: dict[str, object], model_type: type[BaseModel], source: Mapping[str, object]) -> None: diff --git a/src/nemo_safe_synthesizer/configurator/parameter_paths.py b/src/nemo_safe_synthesizer/configurator/parameter_paths.py index 04646eb22..2419ea9f9 100644 --- a/src/nemo_safe_synthesizer/configurator/parameter_paths.py +++ b/src/nemo_safe_synthesizer/configurator/parameter_paths.py @@ -139,11 +139,9 @@ def resolve(self, name: str, *, infer_bare_name: bool = True) -> ParameterNameRe return ResolvedParameterName(requested) return _resolution_from_candidates(name, self._alias_candidates(name)) - top_level = next((field.path for field in self.fields if field.path.parts == (name,)), None) - if top_level is not None: + if (top_level := next((field.path for field in self.fields if field.path.parts == (name,)), None)) is not None: return ResolvedParameterName(top_level) - aliases = self._alias_candidates(name) - if aliases: + if aliases := self._alias_candidates(name): return _resolution_from_candidates(name, aliases) if not infer_bare_name: return UnknownParameterName(name) diff --git a/src/nemo_safe_synthesizer/configurator/parameters.py b/src/nemo_safe_synthesizer/configurator/parameters.py index 019a643c8..9c0e702bd 100644 --- a/src/nemo_safe_synthesizer/configurator/parameters.py +++ b/src/nemo_safe_synthesizer/configurator/parameters.py @@ -183,8 +183,7 @@ def _iter_parameters(self, recursive: bool = True) -> Generator[Mapping[str, Any def _iter_field_paths(self, prefix: tuple[str, ...] = ()) -> Iterator[tuple[tuple[str, ...], Any]]: """Yield every field path and value in this parameter tree.""" for name in type(self).model_fields: - value = self.__dict__.get(name, _MISSING) - if value is _MISSING: + if (value := self.__dict__.get(name, _MISSING)) is _MISSING: continue path = (*prefix, name) yield path, value @@ -197,11 +196,22 @@ def _get_field_path(self, path: tuple[str, ...]) -> object: for part in path: if not isinstance(value, Parameters) or part not in type(value).model_fields: return _MISSING - value = value.__dict__.get(part, _MISSING) - if value is _MISSING: + if (value := value.__dict__.get(part, _MISSING)) is _MISSING: return _MISSING return value + def _matching_field_paths(self, name: str) -> list[tuple[tuple[str, ...], Any]]: + """Return every ``(path, value)`` whose bare field name is ``name``. + + Raises: + ParameterError: If more than one field in the tree matches ``name``. + """ + matches = [(path, value) for path, value in self._iter_field_paths() if path[-1] == name] + if len(matches) > 1: + candidates = ", ".join(format_parameter_path(path) for path, _ in matches) + raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") + return matches + def __iter__(self) -> Iterator[Mapping[str, Any]]: # ty: ignore[invalid-method-override] -- intentionally overrides pydantic BaseModel.__iter__ with parameter-group semantics """Iterate over all parameters, recursing into nested groups.""" return self._iter_parameters(recursive=True) @@ -224,13 +234,11 @@ def get(self, name: str, default: Any = None) -> DataT | Any | None: value = self._get_field_path(tuple(name.split(PARAMETER_PATH_SEPARATOR))) return default if value is _MISSING else value - matches = [(path, value) for path, value in self._iter_field_paths() if path[-1] == name] + matches = self._matching_field_paths(name) if not matches: return default - if len(matches) > 1: - candidates = ", ".join(format_parameter_path(path) for path, _ in matches) - raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") - return matches[0][1] + _, value = matches[0] + return value def has(self, name: str) -> bool: """Check whether ``name`` exists anywhere in the parameter tree. @@ -247,11 +255,7 @@ def has(self, name: str) -> bool: """ if PARAMETER_PATH_SEPARATOR in name: return self._get_field_path(tuple(name.split(PARAMETER_PATH_SEPARATOR))) is not _MISSING - matches = [path for path, _ in self._iter_field_paths() if path[-1] == name] - if len(matches) > 1: - candidates = ", ".join(format_parameter_path(path) for path in matches) - raise ParameterError(f"Ambiguous parameter name {name!r}; use one of: {candidates}.") - return bool(matches) + return bool(self._matching_field_paths(name)) @classmethod def from_yaml_str(cls, raw: str) -> Self: diff --git a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py index 1736e8a6f..76bc42771 100644 --- a/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py +++ b/src/nemo_safe_synthesizer/configurator/pydantic_click_options.py @@ -33,6 +33,9 @@ __all__ = ["pydantic_options", "parse_overrides", "AutoParamType"] +_NEGATION_PREFIX = "no_" +"""Prefix marking a generated disable flag for a nullable sub-config field.""" + _LEGACY_CLI_OPTION_PATHS: dict[str, tuple[str, ...]] = { "generation.structured_generation.enabled": ("generation.use_structured_generation",), "generation.structured_generation.backend": ("generation.structured_generation_backend",), @@ -66,9 +69,11 @@ def parse_overrides(values: dict[str, Any] | None = None, field_sep: str = "__") return {} overrides: dict[str, Any] = {} for k, v in values.items(): - if k.startswith("no_") and isinstance(v, bool): + if k.startswith(_NEGATION_PREFIX) and isinstance(v, bool): if v: - insert_parameter_value(overrides, split_parameter_path(k[3:], field_sep), None) + insert_parameter_value( + overrides, split_parameter_path(k.removeprefix(_NEGATION_PREFIX), field_sep), None + ) continue if v is None: continue @@ -275,7 +280,7 @@ def _collect_params(cls: type[BaseModel], prefix: str = "") -> list[ClickParam]: model_arg = _nullable_model_arg(get_args(t)) if model_arg is not None: params.extend(_collect_params(model_arg, f"{full}.")) - params.append(FlagParam(f"no_{full}", full)) + params.append(FlagParam(f"{_NEGATION_PREFIX}{full}", full)) else: params.append(LeafParam(full, field)) case _: