refactor(config): centralize sparse config patch validation#609
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR shifts configuration handling to schema-aware patches and canonical parameter paths, updates SDK and CLI entry points to consume raw mappings through typed constructors, and expands docs and tests for dotted names, aliasing, ambiguity, and runtime override behavior. ChangesPatch-based config handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e51202e to
236ccf4
Compare
Greptile SummaryThis PR centralizes sparse config patch validation into
Confidence Score: 5/5Safe to merge. The refactoring replaces multiple ad-hoc dict-merge paths with a single well-tested patch compiler; all existing semantics are preserved or deliberately improved with corresponding test coverage. The two new modules are covered by extensive unit tests verifying conflict detection, precedence ordering, alias normalization, and model_fields_set restoration round-trips. The with_runtime_overrides rewrite is exercised by the new TestWithRuntimeOverrides suite. The autoconfig.py change is semantically equivalent to the old merge_dicts path. The only observable behavioral changes are deliberate and consistent with long-standing docstring intent. No files require special attention. The _restore_model_fields_set function in config/patch.py mutates Pydantic's internal model_fields_set set directly — safe because it operates only on freshly-validated model instances — but worth keeping in mind if the team upgrades Pydantic to a version that changes how that set is exposed. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
92c88ac to
2693728
Compare
mckornfield
left a comment
mckornfield
left a comment
There was a problem hiding this comment.
did this PR get mashed together with the ty one? realized I was reviewing something I'd already seen lol. let me know when you have the other one merged and I can stamp this one
b38bbc1 to
8771759
Compare
coderabbitai
Bot
added
refactor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3412ccef-ae01-432b-af69-8ca09d40f3ea
📒 Files selected for processing (8)
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/utils.pytests/config/test_parameters.pytests/sdk/test_config_builder.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported
**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests
**/*.py: UseBaseSettingsfor env/CLI settings, preferAliasChoicesfor per-field dual naming, and useenv_prefixonly for simple settings classes with a shared prefix.
UseField(description=...)as the canonical field docstring for Pydantic models, and always include it.
Use assignment-styleField(default=..., description="...")as the default for model fields; prefer it overAnnotatedunless extra metadata is needed.
UseAnnotatedonly when the field carries additional metadata beyondField()(for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
WhenAnnotatedis used, place defaults as bare assignments (= value), exceptdefault_factory, which should still use assignment-styleField(default_factory=...).
For immutable value objects and validators, prefer@dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults; never use= [].
UseStrEnumfor string-valued enums used in configs or serialization; use plainEnumfor internal-only named constants.
Useobservability.get_logger(__name__)for logging; do not calllogging.getLogger()orstructlog.get_logger()direc...
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for.py,.sh,.yaml, and.yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
tests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/
Files:
src/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insidesrc/, for examplefrom ..observability import get_logger.
Every directory undersrc/that contains Python files must include an__init__.pyfile.
Files:
src/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/config_builder.py
src/nemo_safe_synthesizer/config/**/*.py
⚙️ CodeRabbit configuration file
Treat config changes as user-facing API changes. Check Pydantic field descriptions, defaults, validators, aliases, override behavior, CLI help text impact, YAML compatibility, and documented parameter semantics.
Files:
src/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/config/parameters.py
src/nemo_safe_synthesizer/configurator/**/*.py
⚙️ CodeRabbit configuration file
Review Pydantic-to-Click mapping carefully. Check option names, type conversion, nullable sub-config behavior, validation errors, help text, and compatibility with parse_overrides().
Files:
src/nemo_safe_synthesizer/configurator/parameters.py
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/sdk/test_config_builder.pytests/config/test_parameters.py
🪛 Ruff (0.15.18)
tests/config/test_parameters.py
[warning] 147-147: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 176-176: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 181-181: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 235-235: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 250-250: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🔇 Additional comments (8)
src/nemo_safe_synthesizer/utils.py (1)
16-16: LGTM!Also applies to: 263-272
src/nemo_safe_synthesizer/config/parameters.py (1)
8-11: LGTM!Also applies to: 28-46, 73-104, 234-273, 276-311
src/nemo_safe_synthesizer/configurator/parameters.py (1)
34-42: LGTM!Also applies to: 114-185, 254-254
tests/config/test_parameters.py (1)
4-15: LGTM!Also applies to: 139-144, 185-229, 239-244, 254-263
src/nemo_safe_synthesizer/sdk/config_builder.py (1)
9-9: LGTM!Also applies to: 43-58, 102-131, 139-291
tests/sdk/test_config_builder.py (1)
1-58: LGTM!src/nemo_safe_synthesizer/cli/utils.py (1)
27-27: LGTM!Also applies to: 435-452
src/nemo_safe_synthesizer/config/autoconfig.py (1)
23-23: LGTM!Also applies to: 306-312
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
7b798be to
596c793
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nemo_safe_synthesizer/configurator/parameter_paths.py (1)
240-248: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject alias writes through non-mapping parents.
Line 246 currently turns an existing scalar or
Noneparent into{}, so a legacy alias can hide an explicitly invalid canonical value instead of surfacing a config error. Raise when the branch key already exists but is not a mapping/model.Proposed fix
- for part in path.parts[:-1]: + for index, part in enumerate(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) + elif part in current: + prefix = ".".join(path.parts[: index + 1]) + raise ParameterError( + f"Cannot apply parameter alias {str(path)!r}: {prefix!r} already has a non-mapping value." + ) else: nested = {}As per path instructions, invalid config behavior that hides failures should be prioritized.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b5ed339-0b89-4564-997a-88084794c686
📒 Files selected for processing (20)
STYLE_GUIDE.mddocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/patch.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/utils.pytests/cli/test_utils.pytests/config/test_generate.pytests/config/test_parameters.pytests/config/test_patch.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_config_builder.py
✅ Files skipped from review due to trivial changes (2)
- STYLE_GUIDE.md
- src/nemo_safe_synthesizer/configurator/parameter.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/config/test_generate.py
- tests/configurator/test_pydantic_click_options.py
- src/nemo_safe_synthesizer/utils.py
- tests/cli/test_utils.py
- src/nemo_safe_synthesizer/config/autoconfig.py
- src/nemo_safe_synthesizer/cli/utils.py
- src/nemo_safe_synthesizer/config/parameters.py
- tests/config/test_parameters.py
- src/nemo_safe_synthesizer/configurator/pydantic_click_options.py
- src/nemo_safe_synthesizer/configurator/parameters.py
- docs/user-guide/configuration.md
- tests/config/test_patch.py
- src/nemo_safe_synthesizer/sdk/config_builder.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported
**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/patch.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py:__all__defines the public API surface; identifiers with a leading_are private and may change without notice.
UseBaseSettingsfor env/CLI settings, preferAliasChoicesfor per-field aliases when a field must respond to both its Python name and an env var name, and useenv_prefixonly for simple settings classes with a shared prefix.
Always includeField(description=...)for Pydantic model fields as the canonical field docstring.
Prefer assignment-styleField(...)for Pydantic model fields; useAnnotatedonly when the field carries additional metadata beyondField(), and use a bare assignment for defaults unlessdefault_factoryis needed.
Use@dataclass(frozen=True)for immutable value objects and validators; mutable dataclasses are acceptable for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable dataclass defaults; never use=[].
UseStrEnumfor string-valued enums used in configs/serialization, and plainEnumfor internal-only named constants.
Useobservability.get_logger(__name__)for logging; never calllogging.getLogger()orstructlog.get_logger()directly.
Never useprint()for operational output; useclick.echo()for CLI output orsys.stdout.write()for raw tool output.
Useextra={...}for log data meant for downstream querying or aggregation; use f-strings only for human-readable context.
Raise from the custom error hierarchy (SafeSynthesizerError,UserError,DataError,ParameterError,GenerationError,InternalError) instead of ad hoc exceptions.
Use native Python 3.11 typing syntax (X | Y,list[str],Self,Sequence/Mapping/Iterable,Protocol,TypeIs) and avoid Python 3.12-only syntax such as PEP 695typestatements and bracketed generic parameters.
Guard heavy imports such aspandas,torch, andtransformerswithTYPE_CHECKINGwhen they are only needed for typing.
Prefermatch/casefor dispatch on types or tagged values...
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/patch.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/patch.py
**/*.{py,sh,yaml,yml,md,toml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Every source file must include the repository SPDX copyright and license header, with Markdown using HTML comments unless the file begins with YAML frontmatter.
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Keep files newline-terminated, free of trailing whitespace, with single spaces between sentences and line length within the configured limit.
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.pysrc/nemo_safe_synthesizer/config/generate.pytests/sdk/test_config_builder.pysrc/nemo_safe_synthesizer/config/patch.py
src/nemo_safe_synthesizer/configurator/**/*.py
⚙️ CodeRabbit configuration file
Review Pydantic-to-Click mapping carefully. Check option names, type conversion, nullable sub-config behavior, validation errors, help text, and compatibility with parse_overrides().
Files:
src/nemo_safe_synthesizer/configurator/pydantic_compat.pysrc/nemo_safe_synthesizer/configurator/parameter_paths.py
src/nemo_safe_synthesizer/config/**/*.py
⚙️ CodeRabbit configuration file
Treat config changes as user-facing API changes. Check Pydantic field descriptions, defaults, validators, aliases, override behavior, CLI help text impact, YAML compatibility, and documented parameter semantics.
Files:
src/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/patch.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/sdk/test_config_builder.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/sdk/test_config_builder.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/sdk/test_config_builder.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/sdk/test_config_builder.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/sdk/test_config_builder.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-07-01T20:22:38.492Z
Learning: Use American English spelling in code, docs, configs, and tests (for example, "initialize" not "initialise", "recognize" not "recognise", "color" not "colour").
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/sdk/test_config_builder.py
🔇 Additional comments (6)
tests/sdk/test_config_builder.py (2)
1-252: LGTM!
244-252: 📐 Maintainability & Code QualityNo change needed:
_classify_model_provideris internal state. This test is exercising the intendedresolve()injection contract; there is no public builder API for setting this field.> Likely an incorrect or invalid review comment.src/nemo_safe_synthesizer/configurator/parameter_paths.py (1)
1-237: LGTM!Also applies to: 253-280
src/nemo_safe_synthesizer/configurator/pydantic_compat.py (1)
1-32: LGTM!src/nemo_safe_synthesizer/config/generate.py (1)
7-7: LGTM!Also applies to: 16-16, 275-287
src/nemo_safe_synthesizer/config/patch.py (1)
1-313: LGTM!
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
…lution 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 <aagonzales@nvidia.com>
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 <aagonzales@nvidia.com>
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 <aagonzales@nvidia.com>
2 participants
Summary
Test plan
Related issue: #614
Summary by CodeRabbit