feat(cli): expose env-only infrastructure settings as CLI flags - #538
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:
WalkthroughAdd CLI flags and CLISettings fields for inference/runtime settings; normalize boolean env parsing; propagate CLI-resolved runtime settings into os.environ early; update downstream PII/NER consumers, preflight checks, docs, and tests. ChangesRuntime & environment configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
binaryaaron
added
area:dev-ex
coderabbitai
Bot
added
docs
|
Question for reviewers - shoudl we just rename |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user-guide/environment.md (1)
268-274:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename the closing section to
Next stepsto match doc contract.Use
## Next steps(with the existing links) as the final section title instead of## Related guides.As per coding guidelines: “End documentation pages with 'Next steps' section containing links to related content”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 91d1c4f5-955c-4602-888a-0cbecd8d1dd2
📒 Files selected for processing (13)
docs/user-guide/docker.mddocs/user-guide/environment.mddocs/user-guide/running.mdsrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/cli/test_run.pytests/cli/test_settings.pytests/cli/test_utils.pytests/pii_replacer/test_detect.pytests/test_env_flags.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
docs/user-guide/docker.mdtests/pii_replacer/test_detect.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/run.pydocs/user-guide/environment.mddocs/user-guide/running.mdtests/cli/test_settings.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/docker.mddocs/user-guide/environment.mddocs/user-guide/running.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.md: No decorative**bold**in body text, list items, or docstrings. Use headers, list markers, colons, and backticks for structure.
Use--(em-dash) for asides, not-(hyphen).
Use single backticks for code identifiers, paths, and CLI commands in Markdown.
Use Mermaid diagrams with no spaces in node IDs, quote labels with special characters, no explicit colors or styles.
Include SPDX copyright header in Markdown files using HTML comments:<!-- SPDX-FileCopyrightText: ... -->and<!-- SPDX-License-Identifier: Apache-2.0 -->. Exception: for.mdfiles with YAML frontmatter, include hash-comment headers inside the frontmatter block.All Markdown files require SPDX copyright headers, automatically added by
make format
Files:
docs/user-guide/docker.mddocs/user-guide/environment.mddocs/user-guide/running.md
docs/**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Classify documentation pages as tutorial, how-to, explanation, or reference per the Diataxis framework. Use MkDocs Material syntax for admonitions (
!!! note), tabs (===), and code blocks with titles and highlights.
docs/**/*.md: Classify documentation content using the Diataxis framework (TUTORIAL, HOW-TO, EXPLANATION, or REFERENCE) and ensure each page fits ONE type only
Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for callouts and collapsible content
Use MkDocs Material tab syntax (=== "Tab Name") to present multiple variations or language-specific examples
Include code block metadata in MkDocs Material format: use title attribute for filenames and hl_lines for syntax highlighting of specific lines
Use Mermaid diagram syntax for flowcharts and visual representations in documentation
List prerequisites at the top of each documentation page before main content
End documentation pages with 'Next steps' section containing links to related content
docs/**/*.md: Documentation pages must follow Diataxis framework organization: getting-started/ for tutorials, user-guide/ for how-tos and reference, architecture/ for explanations, reference/ for API docs (auto-generated), dev-notes/ for release notes
Add new documentation pages to thenav:section ofmkdocs.ymlfor sidebar appearance
Use MkDocs Material Markdown extensions including admonitions (!!! note, !!! warning), content tabs (===), code blocks with syntax highlighting, mermaid diagrams, task lists, footnotes, and definition lists
Files:
docs/user-guide/docker.mddocs/user-guide/environment.mddocs/user-guide/running.md
**/*
📄 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:
docs/user-guide/docker.mdtests/pii_replacer/test_detect.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/run.pydocs/user-guide/environment.mddocs/user-guide/running.mdtests/cli/test_settings.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
docs/user-guide/docker.mdtests/pii_replacer/test_detect.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/run.pydocs/user-guide/environment.mddocs/user-guide/running.mdtests/cli/test_settings.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/user-guide/docker.mddocs/user-guide/environment.mddocs/user-guide/running.md
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/pii_replacer/test_detect.pytests/cli/test_run.pytests/test_env_flags.pytests/cli/test_utils.pytests/cli/test_settings.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/pii_replacer/test_detect.pytests/cli/test_run.pytests/test_env_flags.pytests/cli/test_utils.pytests/cli/test_settings.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/pii_replacer/test_detect.pytests/cli/test_run.pytests/test_env_flags.pytests/cli/test_utils.pytests/cli/test_settings.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/pii_replacer/test_detect.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/run.pytests/cli/test_settings.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/pii_replacer/test_detect.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/run.pytests/cli/test_settings.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/pii_replacer/test_detect.pytests/cli/test_run.pytests/test_env_flags.pytests/cli/test_utils.pytests/cli/test_settings.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/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/run.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/data_editor/detect.pysrc/nemo_safe_synthesizer/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/run.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/data_editor/detect.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/pii_replacer/test_detect.pytests/cli/test_run.pytests/test_env_flags.pytests/cli/test_utils.pytests/cli/test_settings.py
🔇 Additional comments (12)
docs/user-guide/docker.md (1)
247-248: LGTM!docs/user-guide/running.md (1)
1236-1236: LGTM!src/nemo_safe_synthesizer/cli/settings.py (1)
30-66: LGTM!Also applies to: 146-221, 231-242
src/nemo_safe_synthesizer/cli/run.py (1)
162-214: LGTM!Also applies to: 379-383, 412-416, 485-489, 514-518, 588-592, 621-625
src/nemo_safe_synthesizer/cli/utils.py (1)
233-238: LGTM!src/nemo_safe_synthesizer/utils.py (1)
30-43: LGTM!src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py (1)
24-25: LGTM!Also applies to: 579-580
tests/cli/test_run.py (1)
307-319: LGTM!tests/cli/test_settings.py (1)
220-281: LGTM!tests/cli/test_utils.py (1)
8-8: LGTM!Also applies to: 16-17, 330-385
tests/pii_replacer/test_detect.py (1)
70-89: LGTM!tests/test_env_flags.py (1)
1-4: LGTM!Also applies to: 6-10, 13-35
Greptile SummaryThis PR exposes five previously env-only infrastructure settings as CLI flags (
Confidence Score: 4/5Safe to merge with awareness: the overall settings-propagation wiring is correct and well-tested, but the new The env-propagation chain, src/nemo_safe_synthesizer/cli/run.py — the
|
| Filename | Overview |
|---|---|
| src/nemo_safe_synthesizer/cli/run.py | Adds five new common_run_options flags; --inference-api-key is a plain string flag that exposes the credential in process listings and shell history. |
| src/nemo_safe_synthesizer/cli/settings.py | Adds five new fields with correct AliasChoices env-var mappings and ge=1 validation on cpu_count; propagation logic is clean. |
| src/nemo_safe_synthesizer/cli/utils.py | Adds _propagate_runtime_settings_to_env that correctly maps CLISettings to os.environ before deferred pii_replacer imports; huggingface_remote correctly maps True→"0"/False→"1" for HF_HUB_OFFLINE. |
| src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py | Converts CONFIG_ID class attribute to config_id() classmethod that reads NSS_INFERENCE_MODEL at call-time, fixing the previously-flagged deferred-import issue. |
| src/nemo_safe_synthesizer/preflight/checks/environment.py | New InferenceModelCheck with correct single-dispatch match logic; replaces InferenceKeyCheck. Priority ordering (endpoint error before key warning before blank model warning) is sound. |
| src/nemo_safe_synthesizer/utils.py | New env_flag_is_true and hf_offline_enabled helpers with correct truthy-value set; well-tested. |
| tests/cli/test_settings.py | Good coverage of new settings fields, env-var loading, CLI override precedence, and cpu_count rejection of non-positive values. |
| script/slurm/slurm_nss_matrix.sh | Renames NIM_MODEL_ID to NSS_INFERENCE_MODEL to align with the new canonical env var; no other changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI invocation\n(--inference-api-key, --inference-model-id,\n--inference-endpoint-url,\n--enable/disable-huggingface-remote,\n--cpu-count)"] --> B["common_run_options()\ncollects Click kwargs"]
B --> C["_settings_from_run_kwargs()\npops CLISettings fields from kwargs"]
C --> D["CLISettings.from_cli_kwargs()\nfilters None, AliasChoices resolves\nenv-var vs CLI precedence"]
D --> E["_propagate_runtime_settings_to_env()\nwrites resolved values to os.environ:\nNSS_INFERENCE_ENDPOINT, NSS_INFERENCE_KEY,\nNSS_INFERENCE_MODEL, HF_HUB_OFFLINE,\nTRANSFORMERS_OFFLINE, NSS_PII_REPLACER_CPU_COUNT"]
E --> F["common_setup() continues:\nWorkdir, Logging, DatasetRegistry, Config"]
F --> G["Deferred pii_replacer imports\nnemo_pii.py reads NSS_INFERENCE_*\nNERFactory reads NSS_PII_REPLACER_CPU_COUNT\nGLiNER reads hf_offline_enabled()"]
G --> H["InferenceModelCheck (preflight)\nreads os.environ at check-time:\nenv.inference validates key/model/endpoint"]
Reviews (8): Last reviewed commit: "chore: update cli help text" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
greptile/coderabbit also has several good comments to address
…* env Hard cutover of the env-only PII/NER runtime settings to a consistent NSS-prefixed scheme, addressing reviewer feedback on PR #538: - Flags: --nim-endpoint-url/-api-key/-model-id -> --inference-endpoint-url/ --inference-api-key/--inference-model-id. - Env: NSS_INFERENCE_ENDPOINT/_KEY/_MODEL, plus new NSS_LOCAL_FILES_ONLY and NSS_CPU_COUNT (replacing NIM_MODEL_ID, LOCAL_FILES_ONLY, SAFE_SYNTHESIZER_CPU_COUNT). - Drop legacy NIM_* aliases and the value-equality precedence shim; pydantic AliasChoices now handles CLI > env precedence directly. - Update downstream readers (column classifier, GLiNER loader, NER worker pool) to the new env names. Also from review: - Enforce cpu_count >= 1 at settings parse time. - Remove redundant type=click.BOOL on the --local-files-only flag pair. - docs: restore model-id default text, NSS_* correspondence, table ordering note, and drop anchors that duplicate MkDocs auto-slugs. BREAKING CHANGE: NIM_* env vars and --nim-* flags are removed; use the NSS_INFERENCE_*/NSS_LOCAL_FILES_ONLY/NSS_CPU_COUNT names and --inference-* flags instead. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
coderabbitai
Bot
added
refactor
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: dd569761-24b0-402e-b1a1-6960d88256c0
📒 Files selected for processing (15)
docs/user-guide/environment.mddocs/user-guide/running.mddocs/user-guide/troubleshooting.mdscript/slurm/slurm_nss_matrix.shsrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/pii_replacer/ner/factory.pytests/cli/test_run.pytests/cli/test_settings.pytests/cli/test_utils.pytests/nss_pii_replacer_test.pytests/pii_replacer/test_detect.pytests/test_env_flags.py
✅ Files skipped from review due to trivial changes (2)
- tests/nss_pii_replacer_test.py
- docs/user-guide/running.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/pii_replacer/test_detect.py
- tests/test_env_flags.py
- src/nemo_safe_synthesizer/cli/utils.py
- tests/cli/test_run.py
- tests/cli/test_utils.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: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{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/troubleshooting.mdsrc/nemo_safe_synthesizer/pii_replacer/ner/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pytests/cli/test_settings.pydocs/user-guide/environment.md
**/*.{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/troubleshooting.mddocs/user-guide/environment.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.md: No decorative**bold**in body text, list items, or docstrings. Use headers, list markers, colons, and backticks for structure.
Use--(em-dash) for asides, not-(hyphen).
Use single backticks for code identifiers, paths, and CLI commands in Markdown.
Use Mermaid diagrams with no spaces in node IDs, quote labels with special characters, no explicit colors or styles.
Include SPDX copyright header in Markdown files using HTML comments:<!-- SPDX-FileCopyrightText: ... -->and<!-- SPDX-License-Identifier: Apache-2.0 -->. Exception: for.mdfiles with YAML frontmatter, include hash-comment headers inside the frontmatter block.All Markdown files require SPDX copyright headers, automatically added by
make format
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/environment.md
docs/**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Classify documentation pages as tutorial, how-to, explanation, or reference per the Diataxis framework. Use MkDocs Material syntax for admonitions (
!!! note), tabs (===), and code blocks with titles and highlights.
docs/**/*.md: Classify documentation content using the Diataxis framework (TUTORIAL, HOW-TO, EXPLANATION, or REFERENCE) and ensure each page fits ONE type only
Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for callouts and collapsible content
Use MkDocs Material tab syntax (=== "Tab Name") to present multiple variations or language-specific examples
Include code block metadata in MkDocs Material format: use title attribute for filenames and hl_lines for syntax highlighting of specific lines
Use Mermaid diagram syntax for flowcharts and visual representations in documentation
List prerequisites at the top of each documentation page before main content
End documentation pages with 'Next steps' section containing links to related content
docs/**/*.md: Documentation pages must follow Diataxis framework organization: getting-started/ for tutorials, user-guide/ for how-tos and reference, architecture/ for explanations, reference/ for API docs (auto-generated), dev-notes/ for release notes
Add new documentation pages to thenav:section ofmkdocs.ymlfor sidebar appearance
Use MkDocs Material Markdown extensions including admonitions (!!! note, !!! warning), content tabs (===), code blocks with syntax highlighting, mermaid diagrams, task lists, footnotes, and definition lists
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/environment.md
**/*
📄 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:
docs/user-guide/troubleshooting.mdscript/slurm/slurm_nss_matrix.shsrc/nemo_safe_synthesizer/pii_replacer/ner/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pytests/cli/test_settings.pydocs/user-guide/environment.md
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
docs/user-guide/troubleshooting.mdscript/slurm/slurm_nss_matrix.shsrc/nemo_safe_synthesizer/pii_replacer/ner/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pytests/cli/test_settings.pydocs/user-guide/environment.md
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/user-guide/troubleshooting.mddocs/user-guide/environment.md
**/*.sh
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.sh: Use shebang#!/usr/bin/env bash(not#!/bin/bash).
Use minimum floorset -eu. Useset -euo pipefailunlesspipefailbreaks piped-grep patterns in the specific script.
Use snake_case for function names and_prefix for internal helper functions.
Always quote variables ("$VAR","${VAR}"). Use defaults via${VAR:-default}. Usereadonlyfor variables that should not change after assignment.
Detect repo root usingREPO_ROOT=${REPO_ROOT:-$(git rev-parse --show-toplevel)}.
Useshellcheckto lint shell scripts. When disabling a check, add# shellcheck disable=SCXXXXwith a brief reason.
Files:
script/slurm/slurm_nss_matrix.sh
⚙️ CodeRabbit configuration file
Review shell scripts for #!/usr/bin/env bash, set -euo pipefail where appropriate, quoting, repo root detection, and shellcheck compliance.
Files:
script/slurm/slurm_nss_matrix.sh
**/*.{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:
script/slurm/slurm_nss_matrix.shsrc/nemo_safe_synthesizer/pii_replacer/ner/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pytests/cli/test_settings.py
**/*.{sh,bash}
📄 CodeRabbit inference engine (AGENTS.md)
Never use
~inside double-quoted strings in shell scripts -- it does not expand. Use$HOMEor an absolute path insteadAll shell scripts require SPDX copyright headers, automatically added by
make format
Files:
script/slurm/slurm_nss_matrix.sh
script/**
⚙️ CodeRabbit configuration file
Review standalone scripts for reproducibility and operational safety. Check argument validation, quoting, repo-root detection, environment variables, generated artifacts, external commands, GPU/cluster assumptions, and whether the script should be wired through Makefile or documented in README/docs.
Files:
script/slurm/slurm_nss_matrix.sh
**/*.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/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pytests/cli/test_settings.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/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.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/factory.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.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/factory.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/cli/test_settings.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_settings.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_settings.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/cli/test_settings.py
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/cli/test_settings.py
🪛 markdownlint-cli2 (0.22.1)
docs/user-guide/environment.md
[warning] 222-222: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (12)
docs/user-guide/troubleshooting.md (1)
548-548: LGTM!script/slurm/slurm_nss_matrix.sh (1)
132-132: LGTM!src/nemo_safe_synthesizer/cli/settings.py (2)
119-124: LGTM!
168-206: LGTM!src/nemo_safe_synthesizer/cli/run.py (4)
164-213: LGTM!
378-382: LGTM!Also applies to: 411-415
484-488: LGTM!Also applies to: 513-517
587-591: LGTM!Also applies to: 620-624
src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py (2)
40-50: LGTM!
579-579: LGTM!src/nemo_safe_synthesizer/pii_replacer/ner/factory.py (1)
150-150: LGTM!tests/cli/test_settings.py (1)
8-9: LGTM!Also applies to: 223-282
coderabbitai
Bot
added
docs
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
coderabbitai
Bot
added
refactor
coderabbitai
Bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd0d87c6-8950-43f7-83bc-f2ab1ab9fd05
📒 Files selected for processing (19)
docs/user-guide/environment.mddocs/user-guide/running.mddocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/cli/run.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/__init__.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/utils.pytests/cli/test_cli_import.pytests/cli/test_run.pytests/cli/test_settings.pytests/cli/test_utils.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/test_env_flags.py
✅ Files skipped from review due to trivial changes (3)
- src/nemo_safe_synthesizer/preflight/init.py
- docs/user-guide/running.md
- docs/user-guide/troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli/test_run.py
- tests/cli/test_settings.py
- src/nemo_safe_synthesizer/utils.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 (14)
**/*.{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/cli/test_cli_import.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pydocs/user-guide/environment.mdsrc/nemo_safe_synthesizer/cli/run.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/cli/test_cli_import.pytests/test_env_flags.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/cli/test_utils.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_cli_import.pytests/test_env_flags.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/cli/test_utils.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_cli_import.pytests/test_env_flags.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/cli/test_utils.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/cli/test_cli_import.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.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/cli/test_cli_import.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
tests/cli/test_cli_import.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pydocs/user-guide/environment.mdsrc/nemo_safe_synthesizer/cli/run.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/cli/test_cli_import.pytests/test_env_flags.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/cli/test_utils.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pydocs/user-guide/environment.mdsrc/nemo_safe_synthesizer/cli/run.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/cli/test_cli_import.pytests/test_env_flags.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/cli/test_utils.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/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.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/telemetry.pysrc/nemo_safe_synthesizer/pii_replacer/data_editor/detect.pysrc/nemo_safe_synthesizer/preflight/checks/__init__.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/cli/utils.pysrc/nemo_safe_synthesizer/cli/settings.pysrc/nemo_safe_synthesizer/cli/run.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/data_editor/detect.py
src/**/__init__.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include
__init__.pyfiles in every directory undersrc/that contains Python files, even if empty, to ensure the directory is recognized as a Python package.
Files:
src/nemo_safe_synthesizer/preflight/checks/__init__.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/environment.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.md: No decorative**bold**in body text, list items, or docstrings. Use headers, list markers, colons, and backticks for structure.
Use--(em-dash) for asides, not-(hyphen).
Use single backticks for code identifiers, paths, and CLI commands in Markdown.
Use Mermaid diagrams with no spaces in node IDs, quote labels with special characters, no explicit colors or styles.
Include SPDX copyright header in Markdown files using HTML comments:<!-- SPDX-FileCopyrightText: ... -->and<!-- SPDX-License-Identifier: Apache-2.0 -->. Exception: for.mdfiles with YAML frontmatter, include hash-comment headers inside the frontmatter block.All Markdown files require SPDX copyright headers, automatically added by
make format
Files:
docs/user-guide/environment.md
docs/**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Classify documentation pages as tutorial, how-to, explanation, or reference per the Diataxis framework. Use MkDocs Material syntax for admonitions (
!!! note), tabs (===), and code blocks with titles and highlights.
docs/**/*.md: Classify documentation content using the Diataxis framework (TUTORIAL, HOW-TO, EXPLANATION, or REFERENCE) and ensure each page fits ONE type only
Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for callouts and collapsible content
Use MkDocs Material tab syntax (=== "Tab Name") to present multiple variations or language-specific examples
Include code block metadata in MkDocs Material format: use title attribute for filenames and hl_lines for syntax highlighting of specific lines
Use Mermaid diagram syntax for flowcharts and visual representations in documentation
List prerequisites at the top of each documentation page before main content
End documentation pages with 'Next steps' section containing links to related content
docs/**/*.md: Documentation pages must follow Diataxis framework organization: getting-started/ for tutorials, user-guide/ for how-tos and reference, architecture/ for explanations, reference/ for API docs (auto-generated), dev-notes/ for release notes
Add new documentation pages to thenav:section ofmkdocs.ymlfor sidebar appearance
Use MkDocs Material Markdown extensions including admonitions (!!! note, !!! warning), content tabs (===), code blocks with syntax highlighting, mermaid diagrams, task lists, footnotes, and definition lists
Files:
docs/user-guide/environment.md
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/user-guide/environment.md
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/cli/test_cli_import.pytests/test_env_flags.pytests/pii_replacer/test_detect.pytests/preflight/test_preflight.pytests/cli/test_utils.py
🪛 Ruff (0.15.15)
tests/cli/test_cli_import.py
[error] 27-27: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (11)
docs/user-guide/environment.md (2)
56-56: LGTM!Also applies to: 76-77, 118-152
133-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBacktick the CLI flags in this heading for consistency.
Sibling headings (
### \HF_HOME`,### `HF_HUB_OFFLINE``) wrap code identifiers in backticks, but this one renders the flags as plain text. This heading isn't referenced by any in-page anchor, so adding backticks won't break links.📝 Proposed fix
-### --enable-huggingface-remote / --disable-huggingface-remote +### `--enable-huggingface-remote` / `--disable-huggingface-remote`As per coding guidelines: "Use single backticks for code identifiers, paths, and CLI commands in Markdown."
> Likely an incorrect or invalid review comment.src/nemo_safe_synthesizer/cli/settings.py (1)
192-202: LGTM!src/nemo_safe_synthesizer/cli/run.py (1)
195-202: LGTM!tests/cli/test_cli_import.py (1)
19-36: LGTM!tests/cli/test_utils.py (2)
347-364: LGTM!
366-375: LGTM!tests/pii_replacer/test_detect.py (1)
70-91: LGTM!tests/preflight/test_preflight.py (1)
32-32: LGTM!Also applies to: 246-324
tests/test_env_flags.py (2)
1-37: LGTM!
40-51: LGTM!
…ffline flag Consolidate inference env validation into a single preflight check and make the Hugging Face offline switch reliable end to end. - preflight: rename InferenceKeyCheck to InferenceModelCheck (env.inference); validate NSS_INFERENCE_KEY, NSS_INFERENCE_MODEL, and NSS_INFERENCE_ENDPOINT via single-dispatch match logic. - cli: replace --local-files-only with --enable/--disable-huggingface-remote (CLI-only, no NSS env var); propagate to HF_HUB_OFFLINE/TRANSFORMERS_OFFLINE. - utils: add shared hf_offline_enabled() and env_flag_is_true(); detect.py reads NSS_INFERENCE_MODEL at call time and GLiNER offline from env. - imports: defer huggingface_hub in telemetry and datasets in utils so the cli.cli import chain stays hub-free; HF_HUB_OFFLINE is then propagated before huggingface_hub first loads. Add tests/cli/test_cli_import regression guard. - docs: document the offline switch, CLI flag precedence, and import-time caching of HF_HUB_OFFLINE. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
8f50068 to
883e40a
Compare
…ndpoint Align runtime and preflight handling of the PII column-classification inference settings: - detect.config_id(): strip NSS_INFERENCE_MODEL; blank/whitespace falls back to DEFAULT_CONFIG_ID instead of sending an empty model id. - nemo_pii._get_classify_endpoint_url(): strip NSS_INFERENCE_ENDPOINT; blank falls back to DEFAULT_NSS_INFERENCE_ENDPOINT instead of passing an empty base_url to the OpenAI client. - preflight env.inference: a non-http(s) NSS_INFERENCE_ENDPOINT is now an error (must not pass --validate), checked before the missing-key and blank-model warnings; a blank endpoint is ignored. Update troubleshooting table and tests accordingly. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
ec8738b
| options.append( | ||
| click.option( | ||
| "--inference-api-key", | ||
| type=str, | ||
| required=False, | ||
| default=None, | ||
| help="API key for the inference endpoint used in PII column classification. " | ||
| "Can also be set via NSS_INFERENCE_KEY env var.", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
--inference-api-key is declared as a plain type=str option. Any value passed on the command line is visible in /proc/PID/cmdline, ps aux, shell history files, and CI logs that echo commands — all readable by other users on the same host. The canonical env-var path (NSS_INFERENCE_KEY) avoids this; the help text documents it, but does not warn that the flag itself is the insecure route. Passing --inference-api-key my-secret on a shared Slurm node or in a verbose CI step will leak the key to any co-tenant process with /proc access.
4 participants
Summary
--inference-endpoint-url,--inference-api-key,--inference-model-id,--enable-huggingface-remote/--disable-huggingface-remote, and--cpu-count.CLISettingswith canonical env names (NSS_INFERENCE_ENDPOINT,NSS_INFERENCE_KEY,NSS_INFERENCE_MODEL,NSS_PII_REPLACER_CPU_COUNT) and propagate toos.environincommon_setupbefore deferredpii_replacerimports.--enable/--disable-huggingface-remoteis CLI-only (no separate NSS env var) and maps to the standardHF_HUB_OFFLINE/TRANSFORMERS_OFFLINEswitches.detect.pyreadsNSS_INFERENCE_MODELat call time, and GLiNER derives offline mode from the env viahf_offline_enabled().InferenceModelCheck(env.inference) validating inference key, model id, and endpoint URL via single-dispatchmatchlogic (replacesInferenceKeyCheck).Test plan
make check(format + ruff + copyright + typecheck)tests/cli/(settings, utils, run help, hub-free import guard)tests/preflight/test_preflight.py(InferenceModelCheck)tests/test_env_flags.py,tests/pii_replacer/test_detect.pytests/cli/test_cli_import.py-- regression guard: importingcli.climust not importhuggingface_hubNotes
pii_replacerto remove the env-propagation shim.tests/cli/test_cli_import.pyguards against a future eagerhuggingface_hub/datasets/transformersimport re-entering thecli.clichain.Summary by CodeRabbit
New Features
Documentation
Behavior
Tests