chore: sweep codeql findings and fix - #505
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📜 Recent 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)
WalkthroughThis PR systematizes Python code quality across 40+ files: standardizing ChangesCode quality improvements: type annotations, imports, and error handling
Test module import isolation and callback testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
coderabbitai
Bot
added
test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60b16746-245b-49c1-906b-716056e459db
📒 Files selected for processing (38)
src/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/config/replace_pii.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/data_processing/records/base.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/training/callbacks.pytests/cli/test_run.pytests/conftest.pytests/e2e/test_dataset_config.pytests/e2e/test_safe_synthesizer.pytests/generation/test_batch.pytests/generation/test_vllm_backend.pytests/observability/test_observability.pytests/preflight/test_plugin_registration.pytests/telemetry/test_telemetry.pytools/codestyle/copyright_fixer.py
💤 Files with no reviewable changes (4)
- src/nemo_safe_synthesizer/config/replace_pii.py
- src/nemo_safe_synthesizer/configurator/parameters.py
- tests/generation/test_vllm_backend.py
- src/nemo_safe_synthesizer/data_processing/records/base.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.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (18)
**/*.{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/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pytests/cli/test_run.pytools/codestyle/copyright_fixer.pytests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/conftest.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pytests/generation/test_batch.pytests/observability/test_observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pytests/preflight/test_plugin_registration.pysrc/nemo_safe_synthesizer/observability.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pytests/cli/test_run.pytools/codestyle/copyright_fixer.pytests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/conftest.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pytests/generation/test_batch.pytests/observability/test_observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pytests/preflight/test_plugin_registration.pysrc/nemo_safe_synthesizer/observability.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pysrc/nemo_safe_synthesizer/observability.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/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pysrc/nemo_safe_synthesizer/observability.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pytests/cli/test_run.pytools/codestyle/copyright_fixer.pytests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/conftest.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pytests/generation/test_batch.pytests/observability/test_observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pytests/preflight/test_plugin_registration.pysrc/nemo_safe_synthesizer/observability.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pytests/cli/test_run.pytools/codestyle/copyright_fixer.pytests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/conftest.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pytests/generation/test_batch.pytests/observability/test_observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pytests/preflight/test_plugin_registration.pysrc/nemo_safe_synthesizer/observability.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/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/configurator/parameter.pytests/cli/test_run.pytools/codestyle/copyright_fixer.pytests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/sdk/config_builder.pytests/conftest.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pytests/generation/test_batch.pytests/observability/test_observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/data_processing/dataset.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pytests/preflight/test_plugin_registration.pysrc/nemo_safe_synthesizer/observability.py
src/nemo_safe_synthesizer/pii_replacer/**/*.py
⚙️ CodeRabbit configuration file
Treat PII replacement changes as high-risk. Check entity coverage, replacement determinism, leakage of original values, handling of empty or multilingual text, and compatibility with optional dependencies.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.py
src/nemo_safe_synthesizer/privacy/**/*.py
⚙️ CodeRabbit configuration file
Treat privacy changes as high-risk. Check DP accounting, parameter validation, data leakage, seed handling, model state persistence, and whether privacy guarantees are documented accurately.
Files:
src/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.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/parameter.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/cli/test_run.pytests/e2e/test_safe_synthesizer.pytests/conftest.pytests/generation/test_batch.pytests/observability/test_observability.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/preflight/test_plugin_registration.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/cli/test_run.pytests/e2e/test_safe_synthesizer.pytests/conftest.pytests/generation/test_batch.pytests/observability/test_observability.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/preflight/test_plugin_registration.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_run.pytests/e2e/test_safe_synthesizer.pytests/conftest.pytests/generation/test_batch.pytests/observability/test_observability.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/preflight/test_plugin_registration.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/cli/test_run.pytests/e2e/test_safe_synthesizer.pytests/conftest.pytests/generation/test_batch.pytests/observability/test_observability.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/preflight/test_plugin_registration.py
tools/**
⚙️ CodeRabbit configuration file
Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.
Files:
tools/codestyle/copyright_fixer.py
tools/codestyle/**
⚙️ CodeRabbit configuration file
Treat codestyle wrappers as CI-critical. Check consistency with Makefile targets, ruff.toml, ty configuration, copyright handling, staged-file behavior, read-only check modes, and whether fixes mutate only expected files.
Files:
tools/codestyle/copyright_fixer.py
src/nemo_safe_synthesizer/evaluation/**/*.py
⚙️ CodeRabbit configuration file
Treat evaluation changes as correctness-sensitive. Check metric inputs, holdout usage, privacy metric semantics, report data shape, missing-data handling, and whether unavailable metrics fail or degrade intentionally.
Files:
src/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/data_processing/dataset.py
tests/conftest.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Shared test fixtures should be defined in
tests/conftest.pyRefer to
tests/conftest.pyfor root-level auto-marking logic,load_test_dataset/load_test_dataframehelpers, andfixture_mock_processorpattern.
Files:
tests/conftest.py
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
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/callbacks.py
🪛 Ruff (0.15.13)
src/nemo_safe_synthesizer/evaluation/statistics/stats.py
[warning] 153-153: Do not catch blind exception: Exception
(BLE001)
src/nemo_safe_synthesizer/data_processing/dataset.py
[warning] 78-78: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (38)
src/nemo_safe_synthesizer/data_processing/records/json_record.py (1)
109-125: LGTM!Also applies to: 131-131, 138-138
tests/cli/test_run.py (1)
6-6: LGTM!Also applies to: 21-22
tests/e2e/test_dataset_config.py (1)
5-5: LGTM!Also applies to: 22-22
tests/e2e/test_safe_synthesizer.py (1)
13-13: LGTM!Also applies to: 30-30
tests/generation/test_batch.py (1)
4-4: LGTM!Also applies to: 17-18, 204-204
tests/observability/test_observability.py (1)
5-5: LGTM!Also applies to: 36-37, 825-827, 831-831
tests/preflight/test_plugin_registration.py (1)
8-8: LGTM!Also applies to: 35-36
tests/telemetry/test_telemetry.py (1)
5-5: LGTM!Also applies to: 28-29
src/nemo_safe_synthesizer/pii_replacer/ner/models.py (1)
198-198: LGTM!tools/codestyle/copyright_fixer.py (1)
202-203: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/custom.py (3)
84-85: LGTM!
149-149: LGTM!
187-187: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/labels.py (1)
33-33: LGTM!Also applies to: 82-82
src/nemo_safe_synthesizer/pii_replacer/ner/model.py (1)
74-74: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/person_name.py (1)
48-48: LGTM!Also applies to: 65-65
src/nemo_safe_synthesizer/cli/artifact_structure.py (2)
26-26: LGTM!Also applies to: 235-235
82-82: LGTM!Also applies to: 174-174, 232-232, 293-293
src/nemo_safe_synthesizer/configurator/parameter.py (1)
98-98: LGTM!src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py (2)
22-22: LGTM!Also applies to: 56-59
621-621: LGTM!src/nemo_safe_synthesizer/cli/run.py (1)
208-213: LGTM!src/nemo_safe_synthesizer/generation/regex_manager.py (1)
290-290: LGTM!Also applies to: 323-323
src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
102-102: LGTM!Also applies to: 188-188, 314-314, 451-452, 456-456
src/nemo_safe_synthesizer/llm/metadata.py (1)
168-168: LGTM!src/nemo_safe_synthesizer/observability.py (1)
150-150: LGTM!Also applies to: 152-152, 165-165
src/nemo_safe_synthesizer/sdk/config_builder.py (1)
129-129: LGTM!src/nemo_safe_synthesizer/data_processing/actions/dates.py (1)
373-373: LGTM!Also applies to: 396-396, 436-436
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py (1)
253-253: LGTM!Also applies to: 442-446
src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py (1)
317-317: LGTM!src/nemo_safe_synthesizer/training/callbacks.py (1)
333-333: LGTM!tests/conftest.py (1)
267-267: LGTM!src/nemo_safe_synthesizer/data_processing/dataset.py (1)
12-13: LGTM!Also applies to: 67-73, 79-80
src/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.py (1)
195-195: LGTM!src/nemo_safe_synthesizer/evaluation/statistics/stats.py (1)
148-148: LGTM!Also applies to: 154-154
src/nemo_safe_synthesizer/pii_replacer/data_editor/edit.py (1)
271-275: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/datetime.py (1)
128-129: LGTM!src/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.py (1)
314-314: LGTM!
coderabbitai
Bot
added
bug
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR sweeps CodeQL findings across the codebase, tightening exception handling, fixing implicit
Confidence Score: 5/5Safe to merge — the changes are mechanical cleanups with no logic regressions found. Every changed code path was reviewed: exception narrowing is appropriate for the APIs in question, the JSONRecord construction refactor preserves the public unpack() contract, and the InferenceEvalCallback loop now correctly exits on the first terminal generation status. The new test in test_callbacks.py validates the break behaviour directly. No incorrect data, lost state, or broken contracts were found. No files require special attention beyond the minor OverflowError gap in stats.py noted in the inline comment. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[on_evaluate triggered] --> B[for _ in range num_batches]
B --> C[generate batch]
C --> D{generation.status}
D -->|IN_PROGRESS| E[continue → next iteration]
E --> B
D -->|STOP_NO_RECORDS| F[control.should_training_stop = True]
D -->|STOP_METRIC_REACHED| G[control.should_training_stop = True]
D -->|other terminal| H[control.should_training_stop = True]
F --> I[log error + append training_incomplete: no_records]
G --> J[log error + append training_incomplete: stopping_condition_reached]
H --> K[no log appended]
I --> L[break]
J --> L
K --> L
L --> M[return]
Reviews (4): Last reviewed commit: "chore: back out a handful per kendrick c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.generation.status == GenerationStatus.IN_PROGRESS: | ||
| continue | ||
|
|
||
| control.should_training_stop = True | ||
| if self.generation.status == GenerationStatus.STOP_NO_RECORDS: | ||
| logger.error( | ||
| "🛑 Stopping generation prematurely. No records were generated. " | ||
| "Please consider adjusting the sampling parameters.", | ||
| ) | ||
| state.log_history.append({"training_incomplete": "no_records"}) # ty: ignore[invalid-argument-type] -- HF Trainer expects dict[str, float] but we use str values for stop signals | ||
| elif self.generation.status == GenerationStatus.STOP_METRIC_REACHED: | ||
| stop_frac = ( | ||
| (self.generation.stop_condition.last_value or 0.0) if self.generation.stop_condition else 0.0 | ||
| ) | ||
| logger.error( | ||
| "🛑 Stopping generation prematurely. The stopping " | ||
| "condition was reached with a running average invalid " | ||
| f"fraction of {stop_frac:.2%}", | ||
| ) | ||
| state.log_history.append({"training_incomplete": "stopping_condition_reached"}) # ty: ignore[invalid-argument-type] -- HF Trainer expects dict[str, float] but we use str values for stop signals |
There was a problem hiding this comment.
break after stop condition is detected
After setting control.should_training_stop = True and logging the stop reason, the refactored code falls through to the end of the loop body without a break. The for _ in range(self.num_batches): loop then continues executing every remaining batch — generating outputs, logging the same stop-reason error message, and calling add_batch — for all iterations that haven't fired yet. The original code issued was_stopped = True; break to exit the loop immediately (even though the subsequent if was_stopped: block was dead code). A break is needed at the end of the stop-handling block to restore that early-exit behaviour and avoid duplicate error messages.
Signed-off-by: mkornfield <mkornfield@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com>
d151a77 to
f0173e9
Compare
mckornfield
changed the title
mckornfield
changed the title
coderabbitai
Bot
added
test
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/nemo_safe_synthesizer/training/callbacks.py (1)
177-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBreak after any terminal generation status.
When
GenerationStatus.STOP_NO_RECORDSis hit, Line 177 already requests trainer shutdown, but this loop still keeps generating remaining batches. That can do extra model work and append multiple"no_records"markers in the same evaluation. Add abreakafter recording the stop reason.Proposed fix
if self.generation.status == GenerationStatus.STOP_NO_RECORDS: logger.error( "🛑 Stopping generation prematurely. No records were generated. " "Please consider adjusting the sampling parameters.", ) state.log_history.append({"training_incomplete": "no_records"}) # ty: ignore[invalid-argument-type] -- HF Trainer expects dict[str, float] but we use str values for stop signals elif self.generation.status == GenerationStatus.STOP_METRIC_REACHED: stop_frac = ( (self.generation.stop_condition.last_value or 0.0) if self.generation.stop_condition else 0.0 ) logger.error( "🛑 Stopping generation prematurely. The stopping " "condition was reached with a running average invalid " f"fraction of {stop_frac:.2%}", ) state.log_history.append({"training_incomplete": "stopping_condition_reached"}) # ty: ignore[invalid-argument-type] -- HF Trainer expects dict[str, float] but we use str values for stop signals - break + break
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2565361e-7aaa-4833-b48e-8e5f5e192d38
📒 Files selected for processing (40)
src/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/config/autoconfig.pysrc/nemo_safe_synthesizer/config/replace_pii.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/configurator/parameters.pysrc/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/data_processing/records/base.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/pii_replacer/ner/person_name.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/training/callbacks.pytests/cli/test_run.pytests/conftest.pytests/e2e/test_dataset_config.pytests/e2e/test_safe_synthesizer.pytests/generation/test_batch.pytests/generation/test_vllm_backend.pytests/observability/test_observability.pytests/preflight/test_plugin_registration.pytests/telemetry/test_telemetry.pytools/codestyle/copyright_fixer.py
💤 Files with no reviewable changes (4)
- src/nemo_safe_synthesizer/config/replace_pii.py
- src/nemo_safe_synthesizer/data_processing/records/base.py
- src/nemo_safe_synthesizer/configurator/parameters.py
- tests/generation/test_vllm_backend.py
✅ Files skipped from review due to trivial changes (9)
- src/nemo_safe_synthesizer/llm/metadata.py
- src/nemo_safe_synthesizer/config/autoconfig.py
- src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
- src/nemo_safe_synthesizer/cli/run.py
- tests/preflight/test_plugin_registration.py
- tests/conftest.py
- src/nemo_safe_synthesizer/pii_replacer/ner/person_name.py
- tools/codestyle/copyright_fixer.py
- tests/observability/test_observability.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.13)
- GitHub Check: Smoke Tests
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{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/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pytests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pytests/generation/test_batch.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pytests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pytests/generation/test_batch.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.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/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pytests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pytests/generation/test_batch.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pytests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pytests/generation/test_batch.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.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/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pytests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/e2e/test_dataset_config.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/pii_replacer/ner/datetime.pytests/generation/test_batch.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/cli/artifact_structure.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/generation/regex_manager.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/configurator/parameter.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/generation/vllm_backend.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/actions/data_actions.pysrc/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/data_processing/records/json_record.pysrc/nemo_safe_synthesizer/data_processing/dataset.py
src/nemo_safe_synthesizer/privacy/**/*.py
⚙️ CodeRabbit configuration file
Treat privacy changes as high-risk. Check DP accounting, parameter validation, data leakage, seed handling, model state persistence, and whether privacy guarantees are documented accurately.
Files:
src/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/cli/test_run.pytests/generation/test_batch.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/cli/test_run.pytests/generation/test_batch.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/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/cli/test_run.pytests/generation/test_batch.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/e2e/test_safe_synthesizer.pytests/telemetry/test_telemetry.pytests/e2e/test_dataset_config.pytests/cli/test_run.pytests/generation/test_batch.py
src/nemo_safe_synthesizer/evaluation/**/*.py
⚙️ CodeRabbit configuration file
Treat evaluation changes as correctness-sensitive. Check metric inputs, holdout usage, privacy metric semantics, report data shape, missing-data handling, and whether unavailable metrics fail or degrade intentionally.
Files:
src/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
src/nemo_safe_synthesizer/pii_replacer/**/*.py
⚙️ CodeRabbit configuration file
Treat PII replacement changes as high-risk. Check entity coverage, replacement determinism, leakage of original values, handling of empty or multilingual text, and compatibility with optional dependencies.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/datetime.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/edit.pysrc/nemo_safe_synthesizer/pii_replacer/ner/model.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
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/regex_manager.pysrc/nemo_safe_synthesizer/generation/vllm_backend.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/parameter.py
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/callbacks.py
🪛 Ruff (0.15.13)
src/nemo_safe_synthesizer/evaluation/statistics/stats.py
[warning] 153-153: Do not catch blind exception: Exception
(BLE001)
src/nemo_safe_synthesizer/data_processing/dataset.py
[warning] 78-78: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (22)
src/nemo_safe_synthesizer/data_processing/actions/data_actions.py (1)
528-528: LGTM!src/nemo_safe_synthesizer/privacy/dp_transformers/privacy_args.py (1)
314-314: LGTM!tests/e2e/test_safe_synthesizer.py (1)
13-13: LGTM!Also applies to: 30-30
tests/telemetry/test_telemetry.py (1)
5-5: LGTM!Also applies to: 28-29
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py (1)
171-171: LGTM!tests/e2e/test_dataset_config.py (1)
5-5: LGTM!Also applies to: 22-22
src/nemo_safe_synthesizer/data_processing/actions/dates.py (1)
373-373: LGTM!Also applies to: 396-396, 436-436
tests/cli/test_run.py (1)
6-6: LGTM!Also applies to: 21-22
src/nemo_safe_synthesizer/sdk/config_builder.py (1)
129-129: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/datetime.py (1)
128-129: LGTM!tests/generation/test_batch.py (1)
4-4: LGTM!Also applies to: 17-17, 204-204
src/nemo_safe_synthesizer/data_processing/records/json_record.py (1)
109-125: LGTM!Also applies to: 131-131, 138-138
src/nemo_safe_synthesizer/cli/artifact_structure.py (1)
26-26: LGTM!Also applies to: 82-82, 174-174, 232-232, 235-235, 293-293
src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py (1)
317-317: LGTM!src/nemo_safe_synthesizer/generation/regex_manager.py (1)
290-290: LGTM!Also applies to: 323-323
src/nemo_safe_synthesizer/evaluation/components/attribute_inference_protection.py (1)
195-195: LGTM!src/nemo_safe_synthesizer/configurator/parameter.py (1)
98-98: LGTM!src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py (1)
253-253: LGTM!Also applies to: 442-446
src/nemo_safe_synthesizer/pii_replacer/ner/custom.py (1)
84-85: LGTM!src/nemo_safe_synthesizer/generation/vllm_backend.py (1)
102-102: LGTM!Also applies to: 188-188, 314-314, 451-453, 456-456
src/nemo_safe_synthesizer/pii_replacer/data_editor/edit.py (1)
271-274: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/model.py (1)
74-74: LGTM!
Signed-off-by: mkornfield <mkornfield@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/training/test_callbacks.py (1)
169-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required test category marker.
The new test has no category marker. Add exactly one (likely
@pytest.mark.unit) to satisfy test marker policy and keep CI selection deterministic.Proposed fix
class TestInferenceEvalCallbackTerminalStatus: """Ensure terminal generation states stop the evaluation loop immediately.""" + `@pytest.mark.unit` def test_callback_stops_after_no_records_status( self, fixture_mock_metadata, fixture_mock_processor, fixture_mock_model,As per coding guidelines, "Marker rules: each test should have exactly one category marker among
unit,smoke,e2e."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b0cffc9-4431-4cd8-93e1-8d3c9a941ae2
📒 Files selected for processing (6)
src/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.pysrc/nemo_safe_synthesizer/training/callbacks.pytests/training/test_callbacks.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: Smoke Tests
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/training/test_callbacks.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/training/test_callbacks.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests in
tests/e2e/should be auto-marked withe2emarker, tests intests/smoke/withsmokemarker, others withunitmarker
tests/**/*.py: Use absolute imports intests/(e.g.,from nemo_safe_synthesizer.observability import get_logger).
Usefixture_prefix convention for fixtures for grep-ability and to separate fixtures from test functions. Add a one-line docstring describing the fixture's purpose and data.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas the primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. If something must be run first before executing a test, include it in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.Organize tests using pytest following the structure in
tests/TESTING.mdwith support for unit tests, smoke tests, and end-to-end tests
Files:
tests/training/test_callbacks.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/training/test_callbacks.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
tests/training/test_callbacks.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
tests/training/test_callbacks.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
tests/training/test_callbacks.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.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/training/test_callbacks.pysrc/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/training/test_callbacks.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.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/data_processing/dataset.pysrc/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/evaluation/statistics/stats.pysrc/nemo_safe_synthesizer/training/callbacks.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/dataset.py
src/nemo_safe_synthesizer/pii_replacer/**/*.py
⚙️ CodeRabbit configuration file
Treat PII replacement changes as high-risk. Check entity coverage, replacement determinism, leakage of original values, handling of empty or multilingual text, and compatibility with optional dependencies.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/custom.pysrc/nemo_safe_synthesizer/pii_replacer/ner/models.py
src/nemo_safe_synthesizer/evaluation/**/*.py
⚙️ CodeRabbit configuration file
Treat evaluation changes as correctness-sensitive. Check metric inputs, holdout usage, privacy metric semantics, report data shape, missing-data handling, and whether unavailable metrics fail or degrade intentionally.
Files:
src/nemo_safe_synthesizer/evaluation/statistics/stats.py
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/callbacks.py
🔇 Additional comments (6)
tests/training/test_callbacks.py (1)
63-80: LGTM!src/nemo_safe_synthesizer/data_processing/dataset.py (1)
67-80: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/custom.py (1)
143-149: LGTM!Also applies to: 181-187
src/nemo_safe_synthesizer/evaluation/statistics/stats.py (1)
62-64: LGTM!Also applies to: 147-154
src/nemo_safe_synthesizer/training/callbacks.py (1)
174-194: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/models.py (1)
199-200: LGTM!
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
Need to look at the epsilon calculation change as I suspect that introduces a bug.
Overall, what's the reasoning for some of these, and I think we should add the common patterns to the STYLE_GUIDE.md.
- importlib usage in tests
- Never using pass on an except (I guess that's the assumption here, but it makes for some odd code)
- Adding raise AssertionError("unreachable") after match statements, even when a wildcard case is present.
| return _type_regex(instance, whitespace_pattern, **kwargs) | ||
| case _: | ||
| raise NotImplementedError() | ||
| raise AssertionError("unreachable") |
There was a problem hiding this comment.
nit: This feels pretty weird to add this raise immediately after a wildcard case.
I guess thinking the cases may change in the future so this raise will always be here. But meh, that's the whole point of the wildcard case. Cleaner to remove the wildcard case and just raise NotImplementedError() as the last line instead maybe?
There was a problem hiding this comment.
yeah... these unreachable statement ones feel like a reach to me, there might be a better way to handle them (or they might just be false positives)
| return result | ||
| case _: | ||
| raise ValueError("input ids are not a tensor or list!") | ||
| raise AssertionError("unreachable") |
There was a problem hiding this comment.
nit: Another where should just put the ValueError raise at the top level and remove the wildcard match if we don't trust/believe the wildcard match.
| eps_R = compute_epsilon(mu_R)[2] | ||
| except (OverflowError, RuntimeError): | ||
| pass | ||
| eps_R = float("inf") |
There was a problem hiding this comment.
suggestion: I don't think this change is correct and should stay as pass (or switch to break). If we've already calculated an eps_R and mu_R, I suspect we want to leave it unchanged (and definitely seems bad to change one but not the other). @andreatgretel can you help confirm.
There was a problem hiding this comment.
I can put it back. I'll just dismiss the empty except warnings, esp if this breaks things
| return cls(**overrides) | ||
| case _: | ||
| raise TypeError(f"Unsupported config type: {type(values)}") | ||
| raise AssertionError("unreachable") |
There was a problem hiding this comment.
nit: Again, let's just not use the match case wildcard then.
| return "plain" | ||
| except (ImportError, AttributeError): | ||
| pass | ||
| return "json" |
There was a problem hiding this comment.
nit: Why do we need to change this?
If we put the return here, then remove the return on 151, right?
| from nemo_safe_synthesizer.telemetry import DeploymentTypeEnum, TaskStatusEnum | ||
| from nemo_safe_synthesizer.tooling import PreflightRenderContext | ||
|
|
||
| importlib.import_module("nemo_safe_synthesizer.sdk.library_builder") # ensure submodule is loaded for mock.patch |
There was a problem hiding this comment.
question: What's more reliable about using importlib vs existing? Using importlib just to remove a # noqa: F401 seems unnecessary to me.
Signed-off-by: mkornfield <mkornfield@nvidia.com>
2 participants
closes #18
make format && make checkor via prek validation.make testpasses locallymake test-e2epasses locallymake test-ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests