feat: move structured generation parameters to an object#595
Conversation
github-actions
Bot
added
area:ci
area:sdk-cli
area:config
area:generation
area:tests
area:docs
labels
|
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:
WalkthroughStructured-generation configuration fields ( ChangesStructured-Generation Configuration Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
docs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR refactors the four flat structured-generation fields on
Confidence Score: 5/5Safe to merge — all callers of the four removed flat fields are updated, three independent compat layers (migration validator, deprecated property stubs with warnings, hidden CLI aliases) guard existing configs and scripts, and the new StructuredGenerationParameters sub-model carries the same validation logic as before. Every code path that previously read or wrote the flat structured-generation fields has been updated. The migration validator, deprecated property stubs, and hidden CLI option aliases form a complete backward-compat net. The notebook JSON fix (single-quote form) addresses the previously reported parsing failure. Tests cover nested construction, flat-key migration, mixed-input precedence, and the CLI legacy aliases. No regressions were identified in the logic. No files require special attention. The migration logic in generate.py and the _section_values helper in parameters.py are the most complex additions but are thoroughly covered by the new TestMixedInputMigration test class. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return values | ||
|
|
||
| @property | ||
| def use_structured_generation(self) -> bool: | ||
| """Deprecated flat alias for ``structured_generation.enabled``.""" | ||
| return self.structured_generation.enabled | ||
|
|
||
| @use_structured_generation.setter | ||
| def use_structured_generation(self, value: bool) -> None: | ||
| self.structured_generation.enabled = value | ||
|
|
||
| @property | ||
| def structured_generation_backend(self) -> StructuredGenerationBackend: | ||
| """Deprecated flat alias for ``structured_generation.backend``.""" | ||
| return self.structured_generation.backend | ||
|
|
||
| @structured_generation_backend.setter | ||
| def structured_generation_backend(self, value: StructuredGenerationBackend) -> None: | ||
| self.structured_generation.backend = value | ||
|
|
||
| @property | ||
| def structured_generation_schema_method(self) -> StructuredGenerationSchemaMethod: | ||
| """Deprecated flat alias for ``structured_generation.schema_method``.""" | ||
| return self.structured_generation.schema_method | ||
|
|
||
| @structured_generation_schema_method.setter | ||
| def structured_generation_schema_method(self, value: StructuredGenerationSchemaMethod) -> None: | ||
| self.structured_generation.schema_method = value | ||
|
|
||
| @property | ||
| def structured_generation_use_single_sequence(self) -> bool: | ||
| """Deprecated flat alias for ``structured_generation.use_single_sequence``.""" | ||
| return self.structured_generation.use_single_sequence | ||
|
|
||
| @structured_generation_use_single_sequence.setter | ||
| def structured_generation_use_single_sequence(self, value: bool) -> None: | ||
| self.structured_generation.use_single_sequence = value |
There was a problem hiding this comment.
DeprecationWarning
The four deprecated property accessors and setters (use_structured_generation, structured_generation_backend, structured_generation_schema_method, structured_generation_use_single_sequence) silently redirect to the new nested model without notifying callers. Existing code that reads or writes these properties — including third-party integrations — will continue working but will never learn that the flat API has been deprecated. A warnings.warn("...", DeprecationWarning, stacklevel=2) in each getter (and setter) would make migration noise visible to users before the aliases are eventually removed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
agent (
review-pr): Deprecated aliases now warn
This looks addressed in the current branch.
| def _section_values(kwargs: dict[str, Any], section: str) -> dict[str, Any]: | ||
| """Return values for a nested section merged with flat compatibility kwargs.""" | ||
| section_value = kwargs.get(section) | ||
| match section_value: | ||
| case BaseModel() as model: | ||
| section_data = model.model_dump() | ||
| case Mapping() as mapping: | ||
| section_data = dict(mapping) | ||
| case None: | ||
| section_data = {} | ||
| case _: | ||
| section_data = {section: section_value} | ||
|
|
||
| flat_values = {key: value for key, value in kwargs.items() if key != section} | ||
| return section_data | flat_values |
There was a problem hiding this comment.
_section_values passes cross-section keys into every sub-model's model_validate
For a call like from_params(generation={"num_records": 100}, training={"batch_size": 8}), the result of _section_values(kwargs, "generation") is {"num_records": 100, "training": {"batch_size": 8}}, so GenerateParameters.model_validate receives training as an unknown field. This is silently dropped only because Parameters uses extra="ignore". The existing design worked the same way with the old model_copy(update=kwargs) path, but switching to model_validate makes the dependency on extra="ignore" load-bearing: a future tightening to extra="forbid" would break every from_params call that mixes section-level dicts with flat kwargs. A brief comment at the call site noting this assumption would prevent a confusing failure later.
There was a problem hiding this comment.
agent (
review-pr): Tracking broader construction cleanup in #596
This concern is about the broader flat from_params(...) construction model rather than the structured-generation object specifically. #596 is already open to clean up the typed config/construction boundaries, so I would avoid expanding #595 into that refactor unless a small piece needs to be pulled forward for compatibility.
For this PR, I would keep the scope to direct migration fixes and focused tests around generation.structured_generation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0abb9a52-1211-4e27-8a5d-9161880ed3aa
📒 Files selected for processing (30)
.github/dco.yamldocs/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mddocs/user-guide/running.mddocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/config/__init__.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/benchmarks/test_generation_structured_methods.pytests/cli/test_utils.pytests/config/conftest.pytests/config/test_generate.pytests/config/test_parameters.pytests/configurator/test_pydantic_click_options.pytests/conftest.pytests/e2e/required_configs/mistral-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/tinyllama-dp.yamltests/e2e/test_dataset_config.pytests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/generation/test_timeseries_backend.pytests/generation/test_vllm_backend.pytests/generation/test_vllm_shutdown.pytests/sdk/test_process_data.pytests/smoke/test_nss_structured_gen_gpu.py
💤 Files with no reviewable changes (1)
- tests/generation/test_timeseries_backend.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). (4)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (25)
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/mistral-dp.yamltests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pytests/sdk/test_process_data.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/mistral-dp.yamltests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pytests/sdk/test_process_data.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) must include SPDX copyright headers. Use mise run format to add them automatically
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamldocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/config/__init__.pytests/e2e/required_configs/mistral-dp.yamldocs/user-guide/troubleshooting.mdtests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pydocs/user-guide/running.mdsrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
YAML configuration files must be checked for consistency. Run mise run check to validate YAML files
**/*.{yaml,yml}: Use 2-space indentation in YAML files.
Use colon-space for key-value pairs in YAML (:).
Include SPDX copyright headers at the top of YAML files.
Use unquoted values in YAML unless special characters require them.
Include a newline at end of YAML files.
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/mistral-dp.yaml
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright headers in all source files using hash-comments for
.py,.sh,.yaml,.ymlfiles.
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamlsrc/nemo_safe_synthesizer/config/__init__.pytests/e2e/required_configs/mistral-dp.yamltests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.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/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamldocs/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/config/__init__.pytests/e2e/required_configs/mistral-dp.yamldocs/user-guide/troubleshooting.mdtests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pydocs/user-guide/running.mdsrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.py
**/*.yaml
⚙️ CodeRabbit configuration file
Review YAML for 2-space indentation, SPDX headers when required, unquoted values unless needed, and newline at EOF.
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/mistral-dp.yaml
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
tests/e2e/required_configs/tinyllama-dp.yamltests/e2e/required_configs/smollm3-dp.yamldocs/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/config/__init__.pytests/e2e/required_configs/mistral-dp.yamldocs/user-guide/troubleshooting.mdtests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pydocs/user-guide/running.mdsrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.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/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mddocs/user-guide/troubleshooting.mddocs/user-guide/running.md
**/*.{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/user-guide/configuration.mdsrc/nemo_safe_synthesizer/config/__init__.pydocs/user-guide/troubleshooting.mdtests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pydocs/user-guide/running.mdsrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.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/user-guide/configuration.mddocs/user-guide/troubleshooting.mddocs/user-guide/running.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 documentationDocumentation markdown files must follow MkDocs Material Markdown extensions syntax including admonitions, content tabs, code blocks with annotations, and Mermaid diagrams
docs/**/*.md: Classify documentation pages indocs/as tutorial, how-to, explanation, or reference per the Diataxis framework.
UseMkDocs Materialsyntax for documentation: admonitions (!!! note), tabs (===), code blocks with titles and highlights.
Files:
docs/user-guide/configuration.mddocs/user-guide/troubleshooting.mddocs/user-guide/running.md
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Markdown documentation must be checked for consistency and formatting. Run mise run check to validate markdown files
**/*.md: No decorative**bold**in markdown body text, list items, or docstrings. Use headers, list markers, colons, and backticks for structure.
Use--(em-dash) for asides in markdown, not-(hyphen).
Use single backticks for code identifiers, paths, and CLI commands in markdown.
Mermaiddiagrams: no spaces in node IDs, quote labels with special characters, no explicit colors or styles.
Use HTML-comment SPDX headers for.mdfiles, or hash-comment headers inside YAML frontmatter when markdown starts with---.
Files:
docs/user-guide/configuration.mddocs/user-guide/troubleshooting.mddocs/user-guide/running.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: Python source files must use ruff for formatting and linting. Run mise run format to auto-fix formatting and linting issues, and mise run check for read-only quality checks
Use ty for Python type checking. Run mise run check to execute type checking on all tracked files
**/*.py: UseField(description=...)as the canonical field docstring for Pydantic models.
Use assignment style (type = Field(default=..., description="...")) as the default for Pydantic model fields rather thanAnnotated-only patterns.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators. Mutable@dataclassacceptable for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults in dataclasses, never= [].
UseStrEnumfor string-valued enums used in configs/serialization. PlainEnumfor internal-only named constants.
UseX | YnotOptional[X]orUnion[X, Y]in type hints.
Uselist[str]notList[str],dict[str, int]notDict[str, int]in type hints.
UseSelffor fluent method returns in type hints.
Use collection ABCs for function arguments (Sequence,Mapping,Iterable) so callers can pass any compatible container; concrete types for return values.
UseProtocolfor structural subtyping when you need duck-typing boundaries.
AvoidAnyin type hints -- preferobject, generics, orProtocol.
UseTYPE_CHECKINGguards for heavy imports (pandas,torch,transformers); not needed for stdlib or lightweight imports.
Prefer `m...
Files:
src/nemo_safe_synthesizer/config/__init__.pytests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pytests/benchmarks/test_generation_structured_methods.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/generation/regex_manager.pytests/sdk/test_process_data.pysrc/nemo_safe_synthesizer/config/generate.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/nemo_safe_synthesizer/**/*.py: Source code must remain Python 3.11 syntax-compatible. Do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Write Python docstrings in Google style format so they auto-generate into API reference pages via mkdocstrings and gen-files plugins
Files:
src/nemo_safe_synthesizer/config/__init__.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/config/generate.py
**/config/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
NSSBaseModelfor config/parameter models inconfig/which define the user-facing configuration of NSS. RawBaseModelor module-specific bases for data transfer objects and internal structures.
Files:
src/nemo_safe_synthesizer/config/__init__.pytests/config/conftest.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_generate.pysrc/nemo_safe_synthesizer/config/generate.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Useobservability.get_logger(__name__)-- 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={}for data that downstream tools should query or aggregate in log calls (metrics, counts, durations). f-strings are fine for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
Prefer clamping/saturation over raising when out-of-range inputs shouldn't crash the system -- return a bounded value with a log warning.
Error messages must precisely match the actual error condition. Use!rfor repr of interpolated pieces to clearly identify them.
Use relative imports insrc/(from ..observability import get_logger), absolute imports intests/(from nemo_safe_synthesizer.observability import get_logger).
Usepathlib.Pathinstead ofos.path. Tolerateos.pathonly in vendored/tooling scripts.
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.print()is fine in tests, standalone scripts, and tooling.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertis fine in tests wherepytestrelies on it.
Files:
src/nemo_safe_synthesizer/config/__init__.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/config/generate.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/config/__init__.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/config/generate.py
src/**/__init__.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Every directory under
src/that contains Python files must include an__init__.pyfile, even if empty.
Files:
src/nemo_safe_synthesizer/config/__init__.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/__init__.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/generate.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pytests/sdk/test_process_data.py
tests/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
tests/**/*.py: New features must include tests before submitting a PR
Bug fixes must include regression tests before submitting a PR
tests/**/*.py: Use file namingtest_*.py, class namingTest*, function namingtest_<module>_<expected_behavior>for test files.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Docstrings are optional for simple tests, recommended for complex/e2e tests explaining purpose.
Markers are auto-assigned by path viapytest_collection_modifyitems(/e2e/->e2e,/smoke/->smoke, default ->unit). Use explicit markers:@pytest.mark.slow,@pytest.mark.requires_gpu,@pytest.mark.timeout().
Usetmp_pathfixture for file operations in tests, never write to the repo tree.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries in tests, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. Include required setup in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.
tests/**/*.py: Assign exactly one of three pytest category markers (unit, smoke, e2e) to every test function
Markers 'slow' and 'requires_gpu' modify the three category markers (unit, smoke, e2e) to indicate when tests should be run or if separate pytest invocations are required
Use 'pytest.mark.noautouse' to skip autouse fixtures for specific tests
Auto-marking via pytest_collection_modifyitems assigns category markers based on test file path: '/e2e/' → 'e2e', '/smoke/' → 'smoke', no match → 'unit', only if no category marker is already present
Use load_test_dataset(filename) from root conftest.py to load HuggingFace Dataset...
Files:
tests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/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/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pytests/sdk/test_process_data.py
tests/**/conftest.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
fixture_prefix convention for test fixtures for grep-ability. Add a one-line docstring describing the fixture's purpose and data.
tests/**/conftest.py: Name dataset and tokenizer fixtures with 'fixture_' prefix (e.g., fixture_iris_dataset, fixture_dow_jones_index_dataset, fixture_tinyllama_config)
Scope tokenizer fixtures as function-scoped since they are expensive to load, and scope most other fixtures as function-scoped except session-scoped cache directories
Convert nullable dtypes to pd.Int64Dtype() or pd.BooleanDtype() before assigning np.nan in test dataframes to avoid Pandas dtype errors
Seed Faker instances with both fake.seed_instance(seed) and random.seed(seed) for reproducible test data generation
Files:
tests/config/conftest.pytests/conftest.py
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/dco.yaml
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/generation/regex_manager.py
tests/conftest.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Store shared test fixtures in
tests/conftest.py
Files:
tests/conftest.py
src/nemo_safe_synthesizer/configurator/**/*.py
⚙️ CodeRabbit configuration file
Review Pydantic-to-Click mapping carefully. Check option names, type conversion, nullable sub-config behavior, validation errors, help text, and compatibility with parse_overrides().
Files:
src/nemo_safe_synthesizer/configurator/pydantic_click_options.py
🧠 Learnings (2)
📚 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/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/config/conftest.pytests/generation/test_vllm_shutdown.pytests/configurator/test_pydantic_click_options.pytests/benchmarks/test_generation_structured_methods.pytests/smoke/test_nss_structured_gen_gpu.pytests/conftest.pytests/e2e/test_dataset_config.pytests/config/test_parameters.pytests/generation/test_vllm_backend.pytests/config/test_generate.pytests/cli/test_utils.pytests/sdk/test_process_data.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/generation/test_regex_manager.pytests/generation/test_vllm_shutdown.pytests/smoke/test_nss_structured_gen_gpu.pytests/generation/test_vllm_backend.py
🔇 Additional comments (19)
docs/tutorials/differential-privacy.ipynb (1)
162-162: LGTM!docs/user-guide/configuration.md (1)
161-164: LGTM!tests/e2e/required_configs/mistral-dp.yaml (1)
8-10: LGTM!tests/e2e/required_configs/smollm3-dp.yaml (1)
8-9: LGTM!tests/e2e/required_configs/tinyllama-dp.yaml (1)
8-10: LGTM!tests/e2e/test_dataset_config.py (1)
147-147: LGTM!tests/e2e/test_safe_synthesizer.py (1)
55-58: LGTM!tests/smoke/test_nss_structured_gen_gpu.py (1)
38-42: LGTM!tests/config/conftest.py (1)
15-15: LGTM!Also applies to: 43-46
tests/config/test_generate.py (1)
11-12: LGTM!Also applies to: 39-119
src/nemo_safe_synthesizer/configurator/pydantic_click_options.py (1)
35-41: LGTM!Also applies to: 320-353
tests/cli/test_utils.py (1)
273-275: LGTM!Also applies to: 302-303, 368-369
tests/benchmarks/test_generation_structured_methods.py (1)
200-209: LGTM!tests/conftest.py (1)
112-113: LGTM!src/nemo_safe_synthesizer/generation/regex_manager.py (1)
363-363: LGTM!Also applies to: 426-426
src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
293-293: LGTM!Also applies to: 347-347, 352-353, 369-369
tests/generation/test_regex_manager.py (1)
29-29: LGTM!Also applies to: 110-110, 171-171, 566-566, 577-577, 595-596, 673-673, 676-676, 698-698, 701-701, 724-724, 729-729
tests/generation/test_vllm_backend.py (1)
103-104: LGTM!Also applies to: 111-113, 120-122, 129-131, 268-268, 300-300, 332-332, 340-340
tests/generation/test_vllm_shutdown.py (1)
38-38: LGTM!
274eeeb to
59057eb
Compare
coderabbitai
Bot
removed
docs
| @@ -159,7 +159,7 @@ | |||
| " .with_data_source(df) # .with_replace_pii(enable=False) to disable PII replacement\n", | |||
There was a problem hiding this comment.
The Jupyter notebook format stores cell source as a JSON array of strings. Python's dict literal {"enabled": True} contains double-quote characters that must be escaped as \" inside a JSON string. The file currently lacks this escaping, making the entire .ipynb file invalid JSON — Jupyter will refuse to open it.
Confirmed: python3 -c "import json,sys; json.load(sys.stdin)" < docs/tutorials/differential-privacy.ipynb exits with a JSONDecodeError. Use single quotes in the Python example ({'enabled': True}) or re-save the notebook through Jupyter which will escape the inner quotes automatically.
There was a problem hiding this comment.
agent (
review-pr): Notebook JSON issue is valid
Agree, this still needs a fix; render it in notebook to verify.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/configurator/test_pydantic_click_options.py (1)
497-508: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExpand legacy CLI alias coverage for all structured-generation compatibility paths.
The legacy alias test currently covers only
backend, but_LEGACY_CLI_OPTION_PATHSdefines three additional aliases (enabled,schema_method,use_single_sequence) that remain untested. A regression in any of those paths would not be caught.Parameterize this test to cover all four structured-generation legacy aliases and assert the correct parsed override structure for each.
Proposed parameterized fix
-def test_structured_generation_legacy_option_end_to_end_via_click_runner(): - """Legacy flat structured-generation CLI aliases remain accepted during migration.""" +@pytest.mark.parametrize( + "legacy_option,expected_key,value", + [ + ("--generation__use_structured_generation", "use_structured_generation", "true"), + ("--generation__structured_generation_backend", "structured_generation_backend", "outlines"), + ("--generation__structured_generation_schema_method", "structured_generation_schema_method", "json"), + ("--generation__structured_generation_use_single_sequence", "structured_generation_use_single_sequence", "true"), + ], +) +def test_structured_generation_legacy_option_end_to_end_via_click_runner(legacy_option, expected_key, value): + """Legacy flat structured-generation CLI aliases remain accepted during migration.""" captured: dict = {} `@pydantic_options`(SafeSynthesizerParameters, field_separator="__") `@click.command`() def cmd(**kwargs): captured.update(parse_overrides(kwargs)) - result = CliRunner().invoke(cmd, ["--generation__structured_generation_backend", "outlines"]) + result = CliRunner().invoke(cmd, [legacy_option, value]) assert result.exit_code == 0, result.output - assert captured["generation"]["structured_generation_backend"] == "outlines" + assert captured["generation"][expected_key] == value
🧹 Nitpick comments (1)
tests/e2e/required_configs/smollm3-dp.yaml (1)
9-9: Clarify the YAML comment intent: it matches e2e behavior (auto for clinc_oos; outlines forced only for dow_jones_index)
smollm3-dp.yamlleavesgeneration.structured_generation.backendunset, andtest_clinc_oos_datasetdoesn’t override it—so the “auto backend works better … on clinc_oos” comment is aligned with current tests.test_dow_jones_index_datasetis the only place that forcesoutlinesforsmollm3-dp.yaml, so clarify that dow_jones_index intentionally uses outlines to prevent confusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40e628a8-5c4f-40fd-ac30-00d61a2fd77e
📒 Files selected for processing (31)
.github/dco.yamldocs/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mddocs/user-guide/running.mddocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/config/__init__.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/telemetry.pytests/benchmarks/test_generation_structured_methods.pytests/cli/test_utils.pytests/config/conftest.pytests/config/test_generate.pytests/config/test_parameters.pytests/configurator/test_pydantic_click_options.pytests/conftest.pytests/e2e/required_configs/mistral-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/tinyllama-dp.yamltests/e2e/test_dataset_config.pytests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/generation/test_timeseries_backend.pytests/generation/test_vllm_backend.pytests/generation/test_vllm_shutdown.pytests/sdk/test_process_data.pytests/smoke/test_nss_structured_gen_gpu.py
💤 Files with no reviewable changes (1)
- tests/generation/test_timeseries_backend.py
✅ Files skipped from review due to trivial changes (8)
- .github/dco.yaml
- docs/user-guide/troubleshooting.md
- docs/user-guide/configuration.md
- tests/smoke/test_nss_structured_gen_gpu.py
- tests/e2e/required_configs/mistral-dp.yaml
- src/nemo_safe_synthesizer/telemetry.py
- tests/conftest.py
- docs/user-guide/running.md
🚧 Files skipped from review as they are similar to previous changes (20)
- docs/tutorials/differential-privacy.ipynb
- src/nemo_safe_synthesizer/config/init.py
- tests/e2e/required_configs/tinyllama-dp.yaml
- tests/generation/test_vllm_shutdown.py
- tests/e2e/test_dataset_config.py
- tests/e2e/test_safe_synthesizer.py
- src/nemo_safe_synthesizer/configurator/pydantic_click_options.py
- src/nemo_safe_synthesizer/config/parameters.py
- src/nemo_safe_synthesizer/sdk/config_builder.py
- tests/benchmarks/test_generation_structured_methods.py
- tests/sdk/test_process_data.py
- tests/config/test_parameters.py
- src/nemo_safe_synthesizer/generation/vllm_backend.py
- tests/config/conftest.py
- src/nemo_safe_synthesizer/generation/regex_manager.py
- tests/generation/test_vllm_backend.py
- tests/generation/test_regex_manager.py
- tests/cli/test_utils.py
- tests/config/test_generate.py
- src/nemo_safe_synthesizer/config/generate.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.{yaml,yml}: Use 2-space indentation in YAML files.
Use colon-space for key-value pairs in YAML (:).
Include SPDX copyright headers at the top of YAML files.
Use unquoted values in YAML unless special characters require them.
Include a newline at end of YAML files.
Files:
tests/e2e/required_configs/smollm3-dp.yaml
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright headers in all source files using hash-comments for
.py,.sh,.yaml,.ymlfiles.
Files:
tests/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
tests/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.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/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.py
**/*.yaml
⚙️ CodeRabbit configuration file
Review YAML for 2-space indentation, SPDX headers when required, unquoted values unless needed, and newline at EOF.
Files:
tests/e2e/required_configs/smollm3-dp.yaml
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
tests/e2e/required_configs/smollm3-dp.yamltests/configurator/test_pydantic_click_options.py
**/*.{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/configurator/test_pydantic_click_options.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: UseField(description=...)as the canonical field docstring for Pydantic models.
Use assignment style (type = Field(default=..., description="...")) as the default for Pydantic model fields rather thanAnnotated-only patterns.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators. Mutable@dataclassacceptable for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults in dataclasses, never= [].
UseStrEnumfor string-valued enums used in configs/serialization. PlainEnumfor internal-only named constants.
UseX | YnotOptional[X]orUnion[X, Y]in type hints.
Uselist[str]notList[str],dict[str, int]notDict[str, int]in type hints.
UseSelffor fluent method returns in type hints.
Use collection ABCs for function arguments (Sequence,Mapping,Iterable) so callers can pass any compatible container; concrete types for return values.
UseProtocolfor structural subtyping when you need duck-typing boundaries.
AvoidAnyin type hints -- preferobject, generics, orProtocol.
UseTYPE_CHECKINGguards for heavy imports (pandas,torch,transformers); not needed for stdlib or lightweight imports.
Prefermatch/casefor dispatch on types or tagged values.if/elifis fine for simple boolean predicates.
Use comprehensions over imperative loops where intent is clearer. No multipleforclauses -- optimize for readability, not conciseness.
Use builder pattern withwith_*methods...
Files:
tests/configurator/test_pydantic_click_options.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/configurator/test_pydantic_click_options.py
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use file namingtest_*.py, class namingTest*, function namingtest_<module>_<expected_behavior>for test files.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Docstrings are optional for simple tests, recommended for complex/e2e tests explaining purpose.
Markers are auto-assigned by path viapytest_collection_modifyitems(/e2e/->e2e,/smoke/->smoke, default ->unit). Use explicit markers:@pytest.mark.slow,@pytest.mark.requires_gpu,@pytest.mark.timeout().
Usetmp_pathfixture for file operations in tests, never write to the repo tree.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries in tests, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. Include required setup in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets from `...
Files:
tests/configurator/test_pydantic_click_options.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/configurator/test_pydantic_click_options.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/configurator/test_pydantic_click_options.py
🔇 Additional comments (2)
tests/e2e/required_configs/smollm3-dp.yaml (1)
8-9: LGTM!tests/configurator/test_pydantic_click_options.py (1)
483-494: LGTM!
59057eb to
6dfe4bd
Compare
coderabbitai
Bot
added
docs
coderabbitai
Bot
removed
docs
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
Would like @binaryaaron to also take a look. The tidying up params into nested fields looks good and a pattern I'd like to continue.
Biggest question is we've had some functionality where passing a single flat dict with keys for various/all subobjects of SafeSynthesizerParameters works, and curious how enabled in particular will interact there. Is that auto setup only work for one layer of subobjects, ie the top level DataParameters, EvaluationParameters, TrainingHyperparams, etc., or does it work for arbitrary nesting. If the latter, can we only have one of these suboejcts with a field named enabled? But maybe this is why examples and code snippets explicitly construct the StructuredGenerationParameters dict/object.
I may also be misremembering how this all works too. But with some magic stuff happening around the Parameters setup, some concrete documentation of what is okay and what isn't seems useful, otherwise I'll be asking these same questions on every related PR forever.
| is built. | ||
| """ | ||
|
|
||
| enabled: Annotated[ |
There was a problem hiding this comment.
question: Do all nested fields need to have unique names for the various broadcasting and auto construction of pydantic model stuff to work?
There was a problem hiding this comment.
had codex run some experiment style things and it came back with
No, nested field names do not need to be globally unique when callers use the nested shape. But flat kwargs are name-based and therefore ambiguous if multiple reachable models expose the same field name. For new nested config groups, we should document that callers should pass the nested object/dict, not an unqualified leaf like enabled.
which I think I'll add the docs for
There was a problem hiding this comment.
agent (
review-pr): Broader construction issue is covered separately
Nested fields do not need globally unique names when callers use the canonical nested shape. The ambiguity is in the legacy flat from_params(...) construction path, where an unqualified leaf like enabled=True can match the wrong section.
I would not ask this PR to solve the whole construction layer. #596 (refactor: improve type-safe config and NER boundaries) is already open and covers the broader typed config/construction cleanup. For #595, I think the right scope is documenting the nested form and fixing any direct structured-generation migration regressions.
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
6f91bf5 to
98c4f3e
Compare
coderabbitai
Bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/dco.yaml (2)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDCO enforcement is weakened for organization members
Line 2 sets
members: false, which exempts members from DCO sign-off checks and conflicts with the repository contribution policy requiring sign-off for all contributions. Smallest fix: setmemberstotrue.Suggested patch
require: - members: false + members: trueAs per coding guidelines,
.github/**must stay aligned with branch-protection/CONTRIBUTING expectations, and CONTRIBUTING requires DCO sign-off for all contributions.Source: Coding guidelines
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing SPDX header in YAML source file
This YAML file is missing the required SPDX hash-comment header, which can fail
mise run check/copyright checks. Smallest fix: add the two SPDX lines at the top.Suggested patch
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + require: members: falseAs per coding guidelines, all
.yamlsource files require SPDX hash-comment headers.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aabf1b17-f567-421a-9080-67949639efda
📒 Files selected for processing (30)
.github/dco.yamldocs/tutorials/differential-privacy.ipynbdocs/user-guide/configuration.mddocs/user-guide/running.mddocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/config/__init__.pysrc/nemo_safe_synthesizer/config/generate.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/configurator/pydantic_click_options.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/benchmarks/test_generation_structured_methods.pytests/cli/test_utils.pytests/config/conftest.pytests/config/test_generate.pytests/config/test_parameters.pytests/configurator/test_pydantic_click_options.pytests/conftest.pytests/e2e/required_configs/mistral-dp.yamltests/e2e/required_configs/smollm3-dp.yamltests/e2e/required_configs/tinyllama-dp.yamltests/e2e/test_dataset_config.pytests/e2e/test_safe_synthesizer.pytests/generation/test_regex_manager.pytests/generation/test_timeseries_backend.pytests/generation/test_vllm_backend.pytests/generation/test_vllm_shutdown.pytests/sdk/test_process_data.pytests/smoke/test_nss_structured_gen_gpu.py
💤 Files with no reviewable changes (1)
- tests/generation/test_timeseries_backend.py
✅ Files skipped from review due to trivial changes (7)
- tests/e2e/required_configs/smollm3-dp.yaml
- tests/generation/test_vllm_shutdown.py
- docs/user-guide/configuration.md
- tests/e2e/required_configs/tinyllama-dp.yaml
- docs/user-guide/running.md
- docs/user-guide/troubleshooting.md
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (16)
- docs/tutorials/differential-privacy.ipynb
- tests/e2e/required_configs/mistral-dp.yaml
- src/nemo_safe_synthesizer/config/init.py
- tests/e2e/test_safe_synthesizer.py
- src/nemo_safe_synthesizer/generation/regex_manager.py
- src/nemo_safe_synthesizer/generation/vllm_backend.py
- tests/smoke/test_nss_structured_gen_gpu.py
- tests/e2e/test_dataset_config.py
- tests/sdk/test_process_data.py
- tests/benchmarks/test_generation_structured_methods.py
- src/nemo_safe_synthesizer/sdk/config_builder.py
- src/nemo_safe_synthesizer/configurator/pydantic_click_options.py
- tests/config/conftest.py
- tests/cli/test_utils.py
- tests/generation/test_regex_manager.py
- tests/generation/test_vllm_backend.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). (4)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (14)
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/dco.yaml
**/*.{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/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.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: UseField(description=...)as the canonical field docstring for Pydantic models.
Use assignment style (type = Field(default=..., description="...")) as the default for Pydantic model fields rather thanAnnotated-only patterns.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators. Mutable@dataclassacceptable for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults in dataclasses, never= [].
UseStrEnumfor string-valued enums used in configs/serialization. PlainEnumfor internal-only named constants.
UseX | YnotOptional[X]orUnion[X, Y]in type hints.
Uselist[str]notList[str],dict[str, int]notDict[str, int]in type hints.
UseSelffor fluent method returns in type hints.
Use collection ABCs for function arguments (Sequence,Mapping,Iterable) so callers can pass any compatible container; concrete types for return values.
UseProtocolfor structural subtyping when you need duck-typing boundaries.
AvoidAnyin type hints -- preferobject, generics, orProtocol.
UseTYPE_CHECKINGguards for heavy imports (pandas,torch,transformers); not needed for stdlib or lightweight imports.
Prefermatch/casefor dispatch on types or tagged values.if/elifis fine for simple boolean predicates.
Use comprehensions over imperative loops where intent is clearer. No multipleforclauses -- optimize for readability, not conciseness.
Use builder pattern withwith_*methods...
Files:
tests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.py
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use file namingtest_*.py, class namingTest*, function namingtest_<module>_<expected_behavior>for test files.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Docstrings are optional for simple tests, recommended for complex/e2e tests explaining purpose.
Markers are auto-assigned by path viapytest_collection_modifyitems(/e2e/->e2e,/smoke/->smoke, default ->unit). Use explicit markers:@pytest.mark.slow,@pytest.mark.requires_gpu,@pytest.mark.timeout().
Usetmp_pathfixture for file operations in tests, never write to the repo tree.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries in tests, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. Include required setup in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets from `...
Files:
tests/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.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/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright headers in all source files using hash-comments for
.py,.sh,.yaml,.ymlfiles.
Files:
tests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
tests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.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/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.py
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
tests/configurator/test_pydantic_click_options.pysrc/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.py
**/config/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use
NSSBaseModelfor config/parameter models inconfig/which define the user-facing configuration of NSS. RawBaseModelor module-specific bases for data transfer objects and internal structures.
Files:
src/nemo_safe_synthesizer/config/parameters.pytests/config/test_parameters.pysrc/nemo_safe_synthesizer/config/generate.pytests/config/test_generate.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Useobservability.get_logger(__name__)-- 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={}for data that downstream tools should query or aggregate in log calls (metrics, counts, durations). f-strings are fine for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
Prefer clamping/saturation over raising when out-of-range inputs shouldn't crash the system -- return a bounded value with a log warning.
Error messages must precisely match the actual error condition. Use!rfor repr of interpolated pieces to clearly identify them.
Use relative imports insrc/(from ..observability import get_logger), absolute imports intests/(from nemo_safe_synthesizer.observability import get_logger).
Usepathlib.Pathinstead ofos.path. Tolerateos.pathonly in vendored/tooling scripts.
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.print()is fine in tests, standalone scripts, and tooling.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertis fine in tests wherepytestrelies on it.
Files:
src/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/generate.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/config/parameters.pysrc/nemo_safe_synthesizer/config/generate.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/
Files:
src/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/generate.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.pysrc/nemo_safe_synthesizer/config/generate.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/configurator/test_pydantic_click_options.pytests/config/test_parameters.pytests/config/test_generate.py
mckornfield
changed the title
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
98c4f3e to
ae26998
Compare
|
The structured-generation object split looks good, and I agree this is a pattern worth continuing for parameter groups that are getting too flat. The main thing to make explicit is the construction behavior. My understanding is that the compatibility On the broader field uniqueness / construction cleanup: I incidentally came across that in #609, which fixes the config/construction layer, so I would avoid expanding this PR into the full refactor unless a small compatibility piece needs to be pulled forward. Prompt for an agent: |
…#607) Code changes was requested by @binaryaaron. * #595 (comment) The following files were modified: * `tests/config/test_generate.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…607) Code changes was requested by @binaryaaron. * #595 (comment) The following files were modified: * `tests/config/test_generate.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
2bd332c to
fd114c3
Compare
3 participants
Summary
Pre-Review Checklist
Ensure that the following pass:
mise run format && mise run checkor via prek validation.mise run testpasses locallymise run test:e2epasses locallymise run 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
Documentation
Chores