fix: allow parameter loading when just rerunning generate - #549
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 extends resume/load behavior: SafeSynthesizer.load_from_save_path accepts an optional runtime_config, warns on saved-vs-current-default drift, merges explicit runtime overrides into saved generation/evaluation/emit_telemetry sections, adjusts CLI and utils resume merge semantics, and adds tests covering these behaviors. ChangesResume-time Configuration Overrides
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
bug
Greptile SummaryThis PR correctly implements field-level deep-merge semantics for the resume (
Confidence Score: 5/5Safe to merge; the field-level merge logic is correct for all reachable CLI and SDK paths, and the existing tests thoroughly cover the override semantics. The core change loads saved configs and applies only explicitly-set runtime fields via a deep recursive merge, correctly handling in-place sub-model mutations that model_dump(exclude_unset=True) would miss. The process_data() early-return on _loaded_from_save_path ensures _resolve_nss_config() never overwrites the merged config in the normal resume flow. Two observations about warning noise and silently-skipped default sub-models are edge cases that do not affect normal CLI or SDK usage. No files require special attention; the two observations in library_builder.py and parameters.py are non-critical edge cases worth a follow-up but not blocking. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if runtime_config is not None: | ||
| saved_config = saved_config.model_copy( | ||
| update={ | ||
| "generation": runtime_config.generation, | ||
| "evaluation": runtime_config.evaluation, | ||
| "emit_telemetry": runtime_config.emit_telemetry, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The entire generation and evaluation objects from runtime_config replace the saved values — not just the fields the user explicitly specified on the CLI. Any saved generation settings that are not re-specified at the CLI (e.g. use_structured_generation=True, temperature, validation params) are silently reset to Pydantic defaults. The new test illustrates this: saved_config.generation.use_structured_generation = True is written to disk, but after load_from_save_path(runtime_config=...) that field is False because runtime_config.generation carries the CLI default — and the test never asserts on it.
A narrower approach is to merge only the fields that the caller explicitly set, using Pydantic's model_fields_set:
if runtime_config is not None:
gen_overrides = runtime_config.generation.model_dump(include=runtime_config.generation.model_fields_set)
eval_overrides = runtime_config.evaluation.model_dump(include=runtime_config.evaluation.model_fields_set)
saved_config = saved_config.model_copy(
update={
"generation": saved_config.generation.model_copy(update=gen_overrides),
"evaluation": saved_config.evaluation.model_copy(update=eval_overrides),
"emit_telemetry": runtime_config.emit_telemetry,
},
)This preserves all saved generation settings while still allowing targeted CLI overrides.
There was a problem hiding this comment.
agent (
review-pr): Fixed in 355c2b6 with field-level merge
Confirmed and addressed. Whole-section replacement reset any saved generation/evaluation field not re-specified at the CLI. Replaced with a field-level merge that applies only explicitly-set runtime values via model_dump(exclude_unset=True), preserving saved values otherwise. Also switched merge_overrides to load config files with exclude_unset=True so partial files no longer materialize defaults as explicit, and skip dataset-registry overrides on resume. New test asserts use_structured_generation=True survives a resume that does not re-specify it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/nemo_safe_synthesizer/sdk/library_builder.py (1)
246-248: ⚡ Quick winClarify docstring to reflect section-level replacement.
The docstring says "runtime_config values for generation and evaluation are applied", which could be interpreted as field-level merging. However, the implementation replaces entire sections. Consider rewording for clarity:
-Optional ``runtime_config`` values for generation and evaluation are -applied after loading the saved training-run config so resume-time CLI -overrides work without mutating the persisted train config. +Optional ``runtime_config`` replaces the entire ``generation``, ``evaluation``, +and ``emit_telemetry`` sections after loading the saved training-run config, +enabling resume-time CLI overrides without mutating the persisted train config. +Callers must ensure ``runtime_config`` is fully populated (all fields resolved), +not just the fields they wish to override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71d5be70-5437-4dc5-8b32-03614f57e6ff
📒 Files selected for processing (4)
src/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_run.pytests/sdk/test_process_data.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
- GitHub Check: Analyze (Python)
- GitHub Check: Typecheck
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/sdk/test_process_data.pytests/cli/test_run.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/sdk/test_process_data.pytests/cli/test_run.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_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/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/sdk/test_process_data.pytests/cli/test_run.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/sdk/test_process_data.pytests/cli/test_run.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/cli/run.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/sdk/test_process_data.pytests/cli/test_run.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/sdk/test_process_data.pytests/cli/test_run.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/sdk/test_process_data.pytests/cli/test_run.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_process_data.pytests/cli/test_run.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/sdk/test_process_data.pytests/cli/test_run.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_process_data.pytests/cli/test_run.py
🔇 Additional comments (2)
src/nemo_safe_synthesizer/cli/run.py (1)
571-571: LGTM!tests/cli/test_run.py (1)
623-650: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Resume-time generate runs replaced whole generation/evaluation config sections with the runtime config, silently resetting any saved field not re-specified at the CLI (e.g. use_structured_generation) to its default. Apply only explicitly-set runtime fields via model_dump(exclude_unset=True), preserving saved values otherwise. merge_overrides now loads config files with exclude_unset=True so partial files no longer materialize defaults as explicit, and dataset-registry overrides are skipped on resume. Warn when saved values drift from current package defaults. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
355c2b6 to
57b5f59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nemo_safe_synthesizer/cli/utils.py (1)
273-274: 💤 Low valueConsider documenting the resume exception in the precedence comment.
The comment block at lines 263-268 explains override precedence but doesn't mention that registry overrides are skipped during resume. Adding a note would help future maintainers understand this behavior without reading the implementation.
📝 Suggested comment addition
# synthesis_overrides collects config overrides from dataset registry and # CLI, which is then combined with the config file when calling # merge_overrides(). CLI takes top precedence, then dataset registry, and # finally the config file. See test_utils.py and especially # test_overrides_config_registry_and_cli for examples of how the resolution # is expected to work. +# Note: During resume mode, registry overrides are skipped to preserve +# saved configuration values. synthesis_overrides: dict[str, Any] | None = dict()tests/cli/test_utils.py (1)
355-368: ⚡ Quick winConsider adding a test case with non-empty overrides.
The current test verifies behavior when
overrides={}. Consider adding a follow-up test that passes non-empty overrides to ensure that both file config fields and override fields are preserved inexclude_unsetoutput, validating the full merge path.🧪 Suggested additional test
def test_partial_config_with_runtime_overrides_preserves_both(self, tmp_path: Path): """Both partial config and runtime overrides should appear in exclude_unset.""" config_file = tmp_path / "config.yaml" config_file.write_text(""" generation: num_records: 77 """) config = merge_overrides(config_file, {"generation": {"temperature": 0.9}}) assert config.generation.num_records == 77 assert config.generation.temperature == 0.9 dumped = config.model_dump(exclude_unset=True) assert dumped == { "generation": { "num_records": 77, "temperature": 0.9, } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 38d40fc0-abb5-43d7-ba46-05c692e8e63a
📒 Files selected for processing (4)
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_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/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.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/cli/utils.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/cli/test_utils.pytests/sdk/test_process_data.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/cli/test_utils.pytests/sdk/test_process_data.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/cli/test_utils.pytests/sdk/test_process_data.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/cli/test_utils.pytests/sdk/test_process_data.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/cli/test_utils.pytests/sdk/test_process_data.py
🧠 Learnings (3)
📚 Learning: 2026-06-01T18:13:50.774Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 538
File: src/nemo_safe_synthesizer/cli/utils.py:0-0
Timestamp: 2026-06-01T18:13:50.774Z
Learning: In `src/nemo_safe_synthesizer/cli/utils.py`, `_propagate_runtime_settings_to_env` intentionally uses write-only (non-`None`) propagation. The function runs exactly once per `common_setup` call in a one-shot CLI process, so there is no prior-invocation leakage risk. A clear-on-`None` guard (else: os.environ.pop(...)) would also be ineffective because `CLISettings` re-reads `os.environ` at construction time, meaning any value written by a hypothetical prior run is already reflected as non-`None` in the field — the else branch would never fire. A real cross-run isolation fix would require snapshotting/restoring `os.environ` around each run, which is out of scope for this PR.
Applied to files:
src/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.py
📚 Learning: 2026-05-14T21:47:20.140Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: tests/TESTING.md:0-0
Timestamp: 2026-05-14T21:47:20.140Z
Learning: Applies to tests/**/tests/**/conftest.py : Use `fixture_` prefix for dataset/tokenizer fixture names. Use descriptive names for non-fixture helpers (e.g., `mock_workdir`).
Applied to files:
tests/cli/test_utils.py
📚 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/cli/test_utils.pytests/sdk/test_process_data.py
🔇 Additional comments (7)
src/nemo_safe_synthesizer/cli/utils.py (1)
400-402: LGTM!tests/cli/test_utils.py (2)
15-15: LGTM!
232-254: ⚡ Quick winTest expectation is correct for the default:
GenerateParameters.num_recordsdefaults to1000, so assertingconfig.generation.num_records == 1000intest_resume_uses_registry_data_without_registry_config_overridesmatches current config defaults.src/nemo_safe_synthesizer/sdk/library_builder.py (3)
57-79: Field-level merge looks correct.
exclude_unset=Trueon the runtime sub-models withmerge_dictsinto the fully-dumped saved section correctly overrides only explicitly-set fields while preserving the rest. This resolves the prior section-level replacement concern.
82-84: Remove concern:_iter_parametersalready guarantees single-key dicts.
Parameters._iter_parameters(recursive=False)constructsparameters = [{k: v} for k, v in self.model_dump().items()]and its docstring explicitly states it yields “single-key dicts”; nested groups use the same_iter_parametersimplementation. Sonext(iter(item.items()))won’t silently drop extra keys under the current contract.
325-328: ⚡ Quick winFix telemetry gate on SDK resume when
emit_telemetryis overridden
load_from_save_path()updatesself._nss_config.emit_telemetry/self._emit_telemetry_config, but_emit_nss_telemetry()uses the value captured during__init__(self._emit_telemetry), soruntime_config=SafeSynthesizerParameters(emit_telemetry=...)during resume may not change whether telemetry is emitted.
- Make
_emit_nss_telemetry()consult the latestself._emit_telemetry_config(orself._nss_config.emit_telemetry) instead of the stale__init__value.- Add a focused test that calls
load_from_save_path(runtime_config=SafeSynthesizerParameters(emit_telemetry=False))and asserts telemetry is skipped.tests/sdk/test_process_data.py (1)
442-578: Solid, behavior-focused coverage of the merge logic.The tests assert concrete preserved/overridden values (including the previously-missing
use_structured_generationpreservation) and cover the no-override, partial-override, and fully-materialized cases plus the drift warning. Mocking is limited to theModelMetadataboundary andtmp_pathis used for IO.
Saved configs carry no provenance, so the drift check flags every saved value that differs from the current default, including deliberate user choices. The prior "differs from the current package default" wording implied a version or default change. Reword to "is non-default" to avoid asserting a package-default change. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Relocate the resume-time override merge from the SDK pipeline to SafeSynthesizerParameters.with_runtime_overrides, reusing utils.merge_dicts. Override detection now walks __pydantic_fields_set__ recursively via _collect_set_fields, so nested sub-group overrides (e.g. generation.validation.*) set by in-place mutation are captured and deep-merged instead of being dropped by model_dump(exclude_unset=True). Document the resume override scope in the run-generate guide and add config-level and resume-path regression tests, including nested generation.validation merges. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
coderabbitai
Bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f30b5c6a-11e4-41d6-9fe4-21209cf3ed2a
📒 Files selected for processing (5)
docs/user-guide/running.mdsrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/sdk/library_builder.pytests/config/test_parameters.pytests/sdk/test_process_data.py
✅ Files skipped from review due to trivial changes (1)
- docs/user-guide/running.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Smoke Tests
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (Python)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pytests/sdk/test_process_data.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pytests/sdk/test_process_data.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.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/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pytests/sdk/test_process_data.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pytests/sdk/test_process_data.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/sdk/library_builder.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pytests/sdk/test_process_data.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/parameters.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/config/test_parameters.pytests/sdk/test_process_data.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/config/test_parameters.pytests/sdk/test_process_data.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/config/test_parameters.pytests/sdk/test_process_data.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/config/test_parameters.pytests/sdk/test_process_data.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/config/test_parameters.pytests/sdk/test_process_data.py
🔇 Additional comments (7)
src/nemo_safe_synthesizer/sdk/library_builder.py (2)
82-82: LGTM!
298-298: LGTM!tests/sdk/test_process_data.py (5)
442-480: LGTM!
482-511: LGTM!
513-545: LGTM!
547-584: LGTM!
586-612: LGTM!
model_copy(update=...) defaults to deep=False, so sections not overridden (data, training, privacy, time_series, preflight, replace_pii) shared mutable references with the original config, and no-op generation/evaluation aliased the original sub-objects. Later mutation of the returned config (e.g. timeseries preprocessing writing config.data.*) could mutate the original, contradicting the method's copy contract. Record only changed sections in the update and use model_copy(deep=True) so unchanged sections are deep-copied; the returned config is now fully independent of self. Add a regression test asserting cross-section isolation. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
coderabbitai
Bot
added
bug
2 participants
Summary
Pre-Review Checklist
Ensure that the following pass:
make format && make checkor via prek validation.make testpasses locallymake test-e2epasses locallymake test-ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation