Skip to content

feat(config): add strict config validation#654

Open
zywind wants to merge 5 commits into
mainfrom
yz/448-strict-config
Open

feat(config): add strict config validation#654
zywind wants to merge 5 commits into
mainfrom
yz/448-strict-config

Conversation

@zywind

@zywind zywind commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject unknown configuration keys recursively by default, with a persisted strict_config: false compatibility opt-out
  • apply the policy consistently across config files, sparse patches, SDK builders, resume, and service payloads
  • document the behavior and cover strict/non-strict validation paths

Test plan

  • mise run check
  • focused config, CLI, SDK, alias, and Click option tests (154 passed)

Closes #448

Summary by CodeRabbit

  • New Features

    • Added strict_config to control recursive unknown configuration key handling (reject vs ignore).
    • Added SDK builder support via with_strict_config(...), including propagation into resolved configs.
    • Extended configuration loading/patching and runtime override merging to preserve effective strictness.
  • Documentation

    • Documented unknown-key behavior, sparse-source handling, and resume-time override semantics for strict_config.
  • Bug Fixes

    • Fixed strictness consistency across nested config/job validation and patch/merge flows, including resume of legacy saved configs.
  • Tests

    • Added/updated CLI, SDK, and configuration tests for strict vs non-strict unknown-key scenarios and resume behavior.

Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2fa7f823-352e-4000-8ab3-e9999a85794b

📥 Commits

Reviewing files that changed from the base of the PR and between 842b9a1 and 22d523c.

📒 Files selected for processing (1)
  • tests/sdk/test_process_data.py
📜 Recent review details
⏰ Context from checks skipped due to timeout. (9)
  • GitHub Check: Smoke Tests
  • GitHub Check: Unit Tests (3.11)
  • GitHub Check: Unit Tests (3.13)
  • GitHub Check: Unit Tests (3.12)
  • GitHub Check: End-user Wheel Install
  • GitHub Check: Greptile Review
  • GitHub Check: Analyze (Python)
  • GitHub Check: Typecheck
  • GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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_process_data.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: Use American English spelling in Python code, documentation, and messages.
Use Field(description=...) for every Pydantic model field.
Use assignment-style Field() by default; use Annotated only for additional metadata such as validators, constrained aliases, or discriminated unions.
Use @dataclass(frozen=True) for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) instead of mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly.
Use .runtime, .user, and .system category loggers appropriately.
Do not use print() for operational library output; use the approved logger, click.echo(), or sys.stdout.write() where appropriate.
Use extra={} for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Prefer X | Y, built-in collection generics, and Self over Optional, Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
Use Protocol for structural subtyping and avoid Any when object, generics, or protocols are suitable.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers.
...

Files:

  • tests/sdk/test_process_data.py
**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Run tests through the repository's mise tasks, such as mise run test, rather than relying on unpinned standalone commands.

Files:

  • tests/sdk/test_process_data.py
tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/e2e, tests/smoke/smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

  • tests/sdk/test_process_data.py
tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to 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 (ruff T201 is suppressed for tests/ directory) and should...

Files:

  • tests/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/sdk/test_process_data.py
**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: All merged commits must use Conventional Commits format: <type>(<scope>): <description> or <type>: <description>, with a lowercase valid type and a description no longer than 100 characters.
All contributions require a DCO Signed-off-by trailer and a cryptographic commit signature.
Before submitting a pull request, run formatting, checks, and tests using mise run format, mise run check, and mise run test.
Branches other than main must use lowercase author-prefixed names in one of the documented forms, optionally including an issue ID and category.
Release tags must use a v prefix and PEP 440 stable or release-candidate versions, such as v1.0.0 or v0.1.0rc0; alpha versions and dashed -rc suffixes are not used.
Do not move a published release tag; create and validate a new release candidate when code changes.
Use mise run <task> for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.

Files:

  • tests/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:

  • tests/sdk/test_process_data.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files with .py, .sh, .yaml, .yml, or .md extensions must include SPDX copyright headers, except files listed in .copyrightignore.

Files:

  • tests/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/sdk/test_process_data.py
🪛 ast-grep (0.44.1)
tests/sdk/test_process_data.py

[info] 529-529: use jsonify instead of json.dumps for JSON output
Context: json.dumps(saved_config)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)

🔇 Additional comments (1)
tests/sdk/test_process_data.py (1)

483-507: LGTM!

Also applies to: 508-545


Walkthrough

Adds strict_config to control recursive unknown-key handling, propagates it through configuration sources, patches, SDK builders, saved configurations, and job validation, and adds documentation and tests for strict and non-strict behavior.

Changes

Configuration strictness

Layer / File(s) Summary
Parameter strictness policy
src/nemo_safe_synthesizer/config/parameters.py, src/nemo_safe_synthesizer/config/job.py
Adds strict_config, resolves Pydantic extras behavior, and applies it during model, patch, runtime, and nested job validation.
Source and SDK propagation
src/nemo_safe_synthesizer/configurator/parameters.py, src/nemo_safe_synthesizer/sdk/...
Propagates unknown-field behavior through raw mappings, configuration sections, builder resolution, and saved configuration loading.
Validation and documentation
tests/config/..., tests/sdk/..., tests/cli/..., docs/...
Covers strict and non-strict validation paths and documents recursive unknown-key behavior, configuration examples, and resume-time overrides.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested labels: feature, docs, test

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.79% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding strict configuration validation.
Linked Issues check ✅ Passed The changes add a configurable strict mode with a default of true, matching the issue's requirements for enable/disable behavior and default strictness.
Out of Scope Changes check ✅ Passed All code and documentation updates are directly tied to strict configuration validation, with no clear unrelated changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yz/448-strict-config

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

zywind added 2 commits July 16, 2026 18:18
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
@zywind
zywind marked this pull request as ready for review July 16, 2026 18:40
@zywind
zywind requested review from a team as code owners July 16, 2026 18:40
@coderabbitai coderabbitai Bot added docs Documentation-only change feature New feature or request test Test-only addition or change labels Jul 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5ef03351-93c2-4be6-9a6f-950e36227939

📥 Commits

Reviewing files that changed from the base of the PR and between 1a33099 and 51b3eb3.

📒 Files selected for processing (11)
  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
  • src/nemo_safe_synthesizer/config/job.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/cli/test_config.py
  • tests/cli/test_utils.py
  • tests/config/test_parameters.py
  • tests/sdk/test_config_builder.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{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:

  • docs/developer-guide/configuration_management.md
  • tests/cli/test_config.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • docs/user-guide/configuration.md
  • tests/sdk/test_config_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • tests/cli/test_utils.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • tests/config/test_parameters.py
**/*.{md,markdown}

📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)

**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use ## headers to segment markdown sections instead of bold text
Use -- (em-dash) instead of - (hyphen) for asides in markdown

Files:

  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
docs/**/*.md

📄 CodeRabbit inference engine (.cursor/rules/writing-docs.mdc)

docs/**/*.md: Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for highlighting important information and collapsible sections in documentation
Use MkDocs Material tabs syntax (=== "Label") to present alternative views or language-specific examples in documentation
Use code block syntax with title and highlight line parameters (title="filename", hl_lines="2 3") for code examples in documentation
Use Mermaid diagram syntax (```mermaid flowchart, etc.) for visualizations in documentation

Classify documentation using Diátaxis and use MkDocs Material syntax for admonitions, tabs, and titled or highlighted code blocks.

Documentation pages must be Markdown files under docs/, placed in the appropriate Diátaxis subdirectory, and added to the nav: section of mkdocs.yml.

Files:

  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
**/*.md

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

Do not use decorative bold in Markdown body text, list items, or docstrings; use single backticks for code identifiers, paths, and commands.

Use the repository's Markdown style and ensure Markdown source files have SPDX copyright headers unless excluded by .copyrightignore.

Files:

  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: All merged commits must use Conventional Commits format: <type>(<scope>): <description> or <type>: <description>, with a lowercase valid type and a description no longer than 100 characters.
All contributions require a DCO Signed-off-by trailer and a cryptographic commit signature.
Before submitting a pull request, run formatting, checks, and tests using mise run format, mise run check, and mise run test.
Branches other than main must use lowercase author-prefixed names in one of the documented forms, optionally including an issue ID and category.
Release tags must use a v prefix and PEP 440 stable or release-candidate versions, such as v1.0.0 or v0.1.0rc0; alpha versions and dashed -rc suffixes are not used.
Do not move a published release tag; create and validate a new release candidate when code changes.
Use mise run <task> for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.

Files:

  • docs/developer-guide/configuration_management.md
  • tests/cli/test_config.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • docs/user-guide/configuration.md
  • tests/sdk/test_config_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • tests/cli/test_utils.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • tests/config/test_parameters.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:

  • docs/developer-guide/configuration_management.md
  • tests/cli/test_config.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • docs/user-guide/configuration.md
  • tests/sdk/test_config_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • tests/cli/test_utils.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • tests/config/test_parameters.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files with .py, .sh, .yaml, .yml, or .md extensions must include SPDX copyright headers, except files listed in .copyrightignore.

Files:

  • docs/developer-guide/configuration_management.md
  • tests/cli/test_config.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • docs/user-guide/configuration.md
  • tests/sdk/test_config_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • tests/cli/test_utils.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • tests/config/test_parameters.py
docs/**

⚙️ CodeRabbit configuration file

Review documentation as MkDocs Material content. Check Diataxis fit, accurate commands, internal links, code fences, and markdown style from STYLE_GUIDE.md.

Files:

  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
**/*.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: Use American English spelling in Python code, documentation, and messages.
Use Field(description=...) for every Pydantic model field.
Use assignment-style Field() by default; use Annotated only for additional metadata such as validators, constrained aliases, or discriminated unions.
Use @dataclass(frozen=True) for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) instead of mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly.
Use .runtime, .user, and .system category loggers appropriately.
Do not use print() for operational library output; use the approved logger, click.echo(), or sys.stdout.write() where appropriate.
Use extra={} for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Prefer X | Y, built-in collection generics, and Self over Optional, Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
Use Protocol for structural subtyping and avoid Any when object, generics, or protocols are suitable.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers.
...

Files:

  • tests/cli/test_config.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/sdk/test_config_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • tests/cli/test_utils.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
  • tests/config/test_parameters.py
**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Run tests through the repository's mise tasks, such as mise run test, rather than relying on unpinned standalone commands.

Files:

  • tests/cli/test_config.py
  • tests/sdk/test_config_builder.py
  • tests/cli/test_utils.py
  • tests/config/test_parameters.py
tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/e2e, tests/smoke/smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

  • tests/cli/test_config.py
  • tests/sdk/test_config_builder.py
  • tests/cli/test_utils.py
  • tests/config/test_parameters.py
tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to 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 (ruff T201 is suppressed for tests/ directory) and should...

Files:

  • tests/cli/test_config.py
  • tests/sdk/test_config_builder.py
  • tests/cli/test_utils.py
  • tests/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/cli/test_config.py
  • tests/sdk/test_config_builder.py
  • tests/cli/test_utils.py
  • tests/config/test_parameters.py
tests/cli/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

Use mock_workdir(tmp_path) helper in CLI tests to create temporary Workdir instances

Files:

  • tests/cli/test_config.py
  • tests/cli/test_utils.py
src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports for package code under src/.
Do not use assert for validation in library code; raise an appropriate exception instead.

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/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.py
  • src/nemo_safe_synthesizer/config/job.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Write durable module-level guidance in Python docstrings and source comments so it appears in the generated API reference.

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • src/nemo_safe_synthesizer/config/job.py
  • src/nemo_safe_synthesizer/configurator/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
src/**/config/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

Use NSSBaseModel for user-facing configuration and parameter models.

Files:

  • src/nemo_safe_synthesizer/config/job.py
  • src/nemo_safe_synthesizer/config/parameters.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/job.py
  • src/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/cli/test_config.py
  • tests/sdk/test_config_builder.py
  • tests/cli/test_utils.py
  • tests/config/test_parameters.py
🪛 Ruff (0.15.21)
src/nemo_safe_synthesizer/config/parameters.py

[warning] 138-138: Do not call setattr with a constant attribute value. It is not any safer than normal property access.

Replace setattr with assignment

(B010)

🔇 Additional comments (10)
src/nemo_safe_synthesizer/config/parameters.py (1)

8-10: LGTM!

Also applies to: 40-42, 99-137, 140-163, 264-319

src/nemo_safe_synthesizer/config/job.py (1)

6-6: LGTM!

Also applies to: 32-39

src/nemo_safe_synthesizer/configurator/parameters.py (1)

34-34: LGTM!

Also applies to: 78-93, 116-116

src/nemo_safe_synthesizer/sdk/config_builder.py (1)

9-9: LGTM!

Also applies to: 57-57, 68-68, 83-91, 114-114, 127-129, 142-144, 157-159, 174-176, 222-226, 243-245, 279-279

src/nemo_safe_synthesizer/sdk/library_builder.py (1)

300-300: LGTM!

tests/config/test_parameters.py (1)

14-14: LGTM!

Also applies to: 110-120, 131-135, 262-375

tests/sdk/test_config_builder.py (1)

89-107: LGTM!

tests/cli/test_config.py (1)

127-144: LGTM!

tests/cli/test_utils.py (1)

378-382: LGTM!

docs/user-guide/configuration.md (1)

22-42: LGTM!

Also applies to: 141-145

Comment thread docs/developer-guide/configuration_management.md
Comment thread src/nemo_safe_synthesizer/config/parameters.py Outdated
Comment thread src/nemo_safe_synthesizer/config/parameters.py
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds strict_config to SafeSynthesizerParameters, defaulting to strict mode (extra="forbid") to catch misspelled or removed configuration keys, with a strict_config: false opt-out for version-skew compatibility. The implementation is thorough — the strictness policy is propagated consistently through the constructor override, model_validate, from_config_source, from_config_patch, with_config_patch, with_runtime_overrides, the SDK builder, and the resume path in library_builder.

  • The default changes from silently ignoring unknown keys to rejecting them recursively; existing configs with any unknown/removed fields (e.g., enable_replace_pii) will need strict_config: false or a cleanup to resume without error.
  • The previously reported resume-path limitation is resolved: the raw JSON is read manually and strict_config_override is injected before validation, so legacy saved configs can be loaded by setting strict_config: false via runtime config or builder.
  • SDK ConfigBuilder tracks _strict_config and _strict_config_explicit to propagate the effective policy consistently through all with_* methods and into the resolved config.

Confidence Score: 4/5

Safe to merge; the implementation is thorough and the previously reported resume-path limitation is correctly resolved. Two minor edge cases around API ordering and double-load semantics are worth noting but do not affect any documented workflow.

The strict-config strictness is applied consistently across all input paths. The prior concern about legacy configs being unloadable on resume is addressed by injecting the override into the raw dict before validation. Two observations remain: the with_strict_config ordering constraint is undocumented, and _strict_config_explicit is set from model_fields_set post-load, which conflates serialized defaults with intentional overrides in repeated-load scenarios. Neither affects the tested workflows.

library_builder.py lines 311-314 for the _strict_config_explicit post-load semantics; config_builder.py with_strict_config docstring for the ordering constraint.

Important Files Changed

Filename Overview
src/nemo_safe_synthesizer/config/parameters.py Core implementation of strict_config; custom init, model_validate, from_config_source, from_config_patch, with_config_patch and with_runtime_overrides all updated consistently.
src/nemo_safe_synthesizer/sdk/library_builder.py Resume path correctly reads raw JSON, optionally injects strict_config_override, then validates. _strict_config and _strict_config_explicit are synced back from the loaded config.
src/nemo_safe_synthesizer/sdk/config_builder.py Adds _strict_config/_strict_config_explicit tracking, with_strict_config() builder method, and propagates _unknown_fields to all from_config_source calls.
src/nemo_safe_synthesizer/config/job.py Adds a field_validator for the nested config field to call SafeSynthesizerParameters.model_validate(), preserving strict_config from the payload.
src/nemo_safe_synthesizer/configurator/parameters.py Parameters base class from_config_source updated to accept and thread unknown_fields through to CompiledConfigPatch for Mapping sources.
tests/config/test_parameters.py Comprehensive coverage of strict/non-strict paths across all public entry points.
tests/sdk/test_process_data.py Parametrized test covers all three policy_source paths for load_from_save_path plus a default-strict rejection test.

Reviews (3): Last reviewed commit: "test(config): cover persisted resume pol..." | Re-trigger Greptile

Comment thread src/nemo_safe_synthesizer/config/parameters.py Outdated
Comment thread src/nemo_safe_synthesizer/config/parameters.py
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
@zywind

zywind commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Greptile resume-path concern in commit 842b9a1. An explicitly set runtime or builder strictness policy now applies before saved JSON validation, with regression tests covering default rejection and both non-strict opt-out paths.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 28e305e4-fd5c-494c-96b8-11aa4cec7ecb

📥 Commits

Reviewing files that changed from the base of the PR and between 51b3eb3 and 842b9a1.

📒 Files selected for processing (7)
  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
  • src/nemo_safe_synthesizer/config/parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/config/test_parameters.py
  • tests/sdk/test_process_data.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • docs/developer-guide/configuration_management.md
  • docs/user-guide/configuration.md
  • tests/config/test_parameters.py
  • src/nemo_safe_synthesizer/sdk/config_builder.py
  • src/nemo_safe_synthesizer/config/parameters.py
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
  • GitHub Check: Unit Tests (3.11)
  • GitHub Check: Unit Tests (3.12)
  • GitHub Check: End-user Wheel Install
  • GitHub Check: Unit Tests (3.13)
  • GitHub Check: Smoke Tests
  • GitHub Check: Greptile Review
  • 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.py
  • tests/sdk/test_process_data.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: Use American English spelling in Python code, documentation, and messages.
Use Field(description=...) for every Pydantic model field.
Use assignment-style Field() by default; use Annotated only for additional metadata such as validators, constrained aliases, or discriminated unions.
Use @dataclass(frozen=True) for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) instead of mutable list defaults.
Use StrEnum for string-valued configuration or serialization enums and plain Enum for internal constants.
Obtain loggers with observability.get_logger(__name__); do not call logging.getLogger() or structlog.get_logger() directly.
Use .runtime, .user, and .system category loggers appropriately.
Do not use print() for operational library output; use the approved logger, click.echo(), or sys.stdout.write() where appropriate.
Use extra={} for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Prefer X | Y, built-in collection generics, and Self over Optional, Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
Use Protocol for structural subtyping and avoid Any when object, generics, or protocols are suitable.
Use TYPE_CHECKING guards for heavy imports such as pandas, torch, and transformers.
...

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/sdk/test_process_data.py
src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports for package code under src/.
Do not use assert for validation in library code; raise an appropriate exception instead.

Files:

  • src/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/sdk/library_builder.py
**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.

**/*: All merged commits must use Conventional Commits format: <type>(<scope>): <description> or <type>: <description>, with a lowercase valid type and a description no longer than 100 characters.
All contributions require a DCO Signed-off-by trailer and a cryptographic commit signature.
Before submitting a pull request, run formatting, checks, and tests using mise run format, mise run check, and mise run test.
Branches other than main must use lowercase author-prefixed names in one of the documented forms, optionally including an issue ID and category.
Release tags must use a v prefix and PEP 440 stable or release-candidate versions, such as v1.0.0 or v0.1.0rc0; alpha versions and dashed -rc suffixes are not used.
Do not move a published release tag; create and validate a new release candidate when code changes.
Use mise run <task> for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/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.py
  • tests/sdk/test_process_data.py
**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files with .py, .sh, .yaml, .yml, or .md extensions must include SPDX copyright headers, except files listed in .copyrightignore.

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
  • tests/sdk/test_process_data.py
src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Write durable module-level guidance in Python docstrings and source comments so it appears in the generated API reference.

Files:

  • src/nemo_safe_synthesizer/sdk/library_builder.py
**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Run tests through the repository's mise tasks, such as mise run test, rather than relying on unpinned standalone commands.

Files:

  • tests/sdk/test_process_data.py
tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/e2e, tests/smoke/smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

  • tests/sdk/test_process_data.py
tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to 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 (ruff T201 is suppressed for tests/ directory) and should...

Files:

  • tests/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/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/sdk/test_process_data.py
🪛 ast-grep (0.44.1)
tests/sdk/test_process_data.py

[info] 499-499: use jsonify instead of json.dumps for JSON output
Context: json.dumps(saved_config)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)


[info] 526-526: use jsonify instead of json.dumps for JSON output
Context: json.dumps(saved_config)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)

🔇 Additional comments (5)
src/nemo_safe_synthesizer/sdk/library_builder.py (3)

8-8: LGTM!


280-284: LGTM!


298-313: LGTM!

tests/sdk/test_process_data.py (2)

15-21: LGTM!


483-507: LGTM!

Comment thread tests/sdk/test_process_data.py
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
@zywind
zywind enabled auto-merge July 17, 2026 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:config area:docs area:sdk-cli area:tests docs Documentation-only change feature New feature or request test Test-only addition or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add strict mode for configs

1 participant