feat: enable transformers v5 + add NVFP4/FP8/MXFP4 quantization schemes - #483
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:
WalkthroughUpgrade to Transformers v5; add a scheme-driven ChangesTransformers v5 Upgrade with Quantization Scheme Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
54478dc to
3e3949b
Compare
d2194a6 to
5b81d84
Compare
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)
CHANGELOG.md (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd SPDX headers to this Markdown file.
This file is missing the required SPDX header block for
.mdsources, which can break repository compliance/format checks.Suggested fix
+<!-- SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> +<!-- SPDX-License-Identifier: Apache-2.0 --> + # ___PROJECT___ 0.0.0 (DD Mon YYYY)As per coding guidelines,
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers.
🧹 Nitpick comments (1)
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py (1)
247-247: ⚡ Quick winRemove the unnecessary
# type: ignorecomment.The type signature of
before_sleep_log(logger, logging.WARNING)matches tenacity's expectedCallable[[RetryCallState], None]contract for thebefore_sleepparameter. The type ignore is not suppressing a real mismatch. Remove it and run type checking to confirm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69db11f4-8a23-42a2-976c-5f54f05e8037
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (12)
CHANGELOG.mddocs/user-guide/configuration.mddocs/user-guide/troubleshooting.mdpyproject.tomlsrc/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pydocs/user-guide/configuration.mdCHANGELOG.mdsrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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/troubleshooting.mddocs/user-guide/configuration.mdCHANGELOG.md
docs/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/writing-docs.mdc)
docs/**/*.md: Classify documentation content using the Diataxis framework (TUTORIAL, HOW-TO, EXPLANATION, or REFERENCE) before writing, ensuring each page fits exactly ONE type
Use cross-links between different Diataxis content types (TUTORIAL, HOW-TO, EXPLANATION, REFERENCE) to connect related documentation
Use MkDocs Material admonitions syntax for notes, warnings, and collapsible tips:!!! note,!!! warning,??? tip
Use MkDocs Material tabs syntax (=== "Label") for presenting multiple examples or implementations side-by-side
Use code blocks with metadata (title, hl_lines) to highlight relevant code snippets in documentation examples
Use Mermaid diagrams (flowchart, sequence diagrams, etc.) for visualizing architecture, workflows, and concepts in documentation
Write documentation following high signal-to-noise principles: every sentence must earn its place by providing essential information
Use progressive disclosure in documentation: start with simple concepts, then layer complexity for advanced readers
Include working code examples in documentation; ensure all code snippets are tested and actually work
List all prerequisites at the top of documentation pages before diving into main content
End documentation pages with 'Next steps' section containing links to related content and logical progression pointsClassify 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: Documentation pages are located indocs/following Diataxis framework with subdirectories: getting-started/ (tutorials), user-guide/ (how-tos and reference), architecture/ (explanations), reference/ (API reference), and blog/ (dev notes)
Markdown documentation should use MkDocs Material features including admonitions (!!! note,!!! warning,??? tip), content tabs, code blocks with syntax highlighting, Mermaid di...
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/configuration.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.
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/configuration.mdCHANGELOG.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.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pydocs/user-guide/configuration.mdCHANGELOG.mdsrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.pypyproject.toml
⚙️ 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.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pydocs/user-guide/configuration.mdCHANGELOG.mdsrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.pypyproject.toml
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (
.py,.sh,.yaml,.yml,.md) require SPDX copyright headers; usemake formatto add them automatically
Files:
docs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pydocs/user-guide/configuration.mdCHANGELOG.mdsrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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/troubleshooting.mddocs/user-guide/configuration.md
**/*.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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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.API reference documentation is auto-generated from Python docstrings using Google-style formatting; write docstrings in
src/nemo_safe_synthesizer/and they will appear in the reference section on next build
Files:
src/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/training/test_huggingface_backend.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/training.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/components/text_semantic_similarity.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/training/test_huggingface_backend.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.Use
make testfor unit tests,make test-unit-slowfor unit tests including slow tests,make test-smokefor CPU smoke tests,make test-smoke-gpufor GPU smoke tests, andmake test-e2efor end-to-end tests before submitting PR
Files:
tests/training/test_huggingface_backend.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_huggingface_backend.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/training/test_huggingface_backend.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/dp_utils.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/huggingface_backend.py
src/nemo_safe_synthesizer/config/**/*.py
⚙️ CodeRabbit configuration file
Treat config changes as user-facing API changes. Check Pydantic field descriptions, defaults, validators, aliases, override behavior, CLI help text impact, YAML compatibility, and documented parameter semantics.
Files:
src/nemo_safe_synthesizer/config/training.py
pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Package metadata, dependencies, and extras (cpu/cu128/engine) should be configured in
pyproject.tomlOrder sections in
pyproject.tomlas:[project],[dependency-groups],[project.optional-dependencies],[tool.uv],[build-system],[tool.*].
Files:
pyproject.toml
⚙️ CodeRabbit configuration file
Treat pyproject.toml as high-risk. Check package metadata, uv indexes, dependency groups, optional extras, Python version bounds, hatch config, ty config, script entry points, dependency consistency, and whether changes require regenerating uv.lock.
Files:
pyproject.toml
**/*.toml
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.toml: Use spaces around=for key-value pairs in TOML files.
Use# commentformat for comments in TOML files; use inline comments for dependency pins.
Files:
pyproject.toml
🪛 LanguageTool
docs/user-guide/configuration.md
[grammar] ~123-~123: Ensure spelling is correct
Context: ...Weight-only NVIDIA FP4. Requires recent torchao. | | mxfp4 | Mxfp4Config | 4 | Vari...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.15.12)
src/nemo_safe_synthesizer/llm/utils.py
[warning] 590-590: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py (1)
26-26: LGTM!Also applies to: 49-49, 571-573, 576-576, 654-657, 672-675
src/nemo_safe_synthesizer/preflight/checks/environment.py (1)
209-215: LGTM!pyproject.toml (1)
28-28: LGTM!Also applies to: 92-92, 120-121, 126-126, 136-136, 143-144, 153-153, 160-160, 189-189
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nemo_safe_synthesizer/llm/utils.py (1)
705-718: ⚡ Quick winRemove or document the
ty:ignore[unresolved-import]onMxfp4Config.The class exists in transformers v5 (web search confirms it is documented for MXFP4 quantization). The suppress is likely a workaround for incomplete type stubs in transformers, not a missing runtime import. Per the style guide,
ty:ignoreshould not be used without an active fix attempt. Either remove the suppress if type stubs have caught up, or add a comment (e.g.,# ty:ignore[unresolved-import] # transformers v5 lacks stubs) explaining the known limitation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ba45f891-8d35-446f-9e5b-dabe8e1bf785
📒 Files selected for processing (5)
src/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/data_processing/test_assembler.pytests/generation/test_processors.pytests/smoke/conftest.py
✅ Files skipped from review due to trivial changes (2)
- tests/data_processing/test_assembler.py
- tests/smoke/conftest.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: Smoke Tests
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/generation/test_processors.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.Use
make testfor unit tests,make test-unit-slowfor unit tests including slow tests,make test-smokefor CPU smoke tests,make test-smoke-gpufor GPU smoke tests, andmake test-e2efor end-to-end tests before submitting PR
Files:
tests/generation/test_processors.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/generation/test_processors.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/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.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/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.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/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.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/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/generation/test_processors.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (
.py,.sh,.yaml,.yml,.md) require SPDX copyright headers; usemake formatto add them automatically
Files:
tests/generation/test_processors.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/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.API reference documentation is auto-generated from Python docstrings using Google-style formatting; write docstrings in
src/nemo_safe_synthesizer/and they will appear in the reference section on next build
Files:
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.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/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/llm/utils.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/dp_utils.py
🪛 Ruff (0.15.13)
src/nemo_safe_synthesizer/llm/utils.py
[warning] 590-590: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
src/nemo_safe_synthesizer/llm/utils.py (3)
573-574: Forceuse_fast=Trueinstead of allowing override.The docstring states "
use_fastis forced toTrue" butsetdefaultallows callers to passuse_fast=False, which would silently bypass the helper's intent.Suggested fix
- kwargs.setdefault("use_fast", True) + kwargs["use_fast"] = True
683-684: Reject invalid legacy bit values instead of silently defaulting to 8-bit.The current logic maps any non-
4integer (e.g.,2,16,32) toBNB_8BIT. This silently accepts invalid inputs and conflicts with theValueErrorbehavior documented for unknown schemes.Suggested fix
if isinstance(scheme, int): - scheme = QuantizationScheme.BNB_4BIT if scheme == 4 else QuantizationScheme.BNB_8BIT + if scheme == 4: + scheme = QuantizationScheme.BNB_4BIT + elif scheme == 8: + scheme = QuantizationScheme.BNB_8BIT + else: + raise ValueError(f"Unknown quantization bit-count alias: {scheme!r}. Expected 4 or 8.")
18-28: LGTM!Also applies to: 538-543, 546-581, 584-591, 594-636, 657-682, 687-704
tests/generation/test_processors.py (1)
6-6: LGTM!Also applies to: 48-48, 463-463
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py (1)
26-26: LGTM!Also applies to: 49-49, 506-508, 548-548, 571-578, 662-662, 679-681
Greptile SummaryThis PR upgrades the safe-synthesizer library to support transformers v5, introducing new quantization schemes (fp8, nvfp4, mxfp4) alongside the existing bitsandbytes options. Several v5 API breaking changes are handled:
Confidence Score: 5/5Safe to merge — all three previously identified blocking issues (invalid bnb_8bit_* kwargs, create_optimizer model-arg mismatch, group_by_length removal) are correctly resolved, and the new quantization dispatch logic is well-tested. The quantization scheme dispatch is clean and each path has targeted unit tests. The DP training changes (create_optimizer signature, tuple-loss guard, gradient-checkpointing model-level disable) address real v5 API shifts. Backward compatibility through the legacy quantization_bits alias is preserved. The only finding is a style inconsistency in how Mxfp4Config is imported. No files require special attention; the one import-style note in config/training.py is cosmetic. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["quantize_model=True"] --> B{"quantization_scheme set?"}
B -- "Yes" --> C["Use QuantizationScheme directly"]
B -- "No" --> D{"quantization_bits?"}
D -- "4" --> E["BNB_4BIT"]
D -- "8" --> F["BNB_8BIT"]
C --> G{"scheme.is_bitsandbytes?"}
E --> G
F --> G
G -- "True + loftq" --> H["LoftQConfig(loftq_bits=bits)"]
G -- "False + loftq" --> I["ParameterError raised"]
G -- "Any + lora/qlora" --> J["to_transformers_config()"]
J --> K{"scheme"}
K -- "bnb-4bit" --> L["BitsAndBytesConfig\nload_in_4bit NF4+double+bf16"]
K -- "bnb-8bit" --> M["BitsAndBytesConfig\nload_in_8bit=True"]
K -- "fp8" --> N["FineGrainedFP8Config()"]
K -- "nvfp4" --> O["TorchAoConfig\nNVFP4WeightOnlyConfig"]
K -- "mxfp4" --> P["Mxfp4Config()"]
L & M & N & O & P --> Q["quantization_config= in from_pretrained"]
Reviews (8): Last reviewed commit: "docs: use valid TOML for vLLM version-se..." | Re-trigger Greptile |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| class NSSTrainingAndGenerationEvent(BaseModel): | ||
| _event_name: ClassVar[str] = "train_and_generation_event" | ||
| _schema_version: ClassVar[str] = "1.4" | ||
| _schema_version: ClassVar[str] = "1.7" |
There was a problem hiding this comment.
this (I hope) will alleviate some of the weirdness on that version of the telemetry, but can do it in its own pr
There was a problem hiding this comment.
Please put this in it's own PR. I promise I will approve it quickly :)
There was a problem hiding this comment.
Reverted the telemetry change from this PR
There was a problem hiding this comment.
This change is still visible in the github console view I'm seeing.
8153a64 to
955c2c0
Compare
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: 13c72857-14ce-477d-a277-45b9d5be1989
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (18)
CHANGELOG.mddocs/user-guide/configuration.mddocs/user-guide/troubleshooting.mdpyproject.tomlsrc/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/llm/utils.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/telemetry.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_assembler.pytests/generation/test_processors.pytests/llm/test_metadata.pytests/llm/test_utils.pytests/smoke/conftest.pytests/training/test_huggingface_backend.py
💤 Files with no reviewable changes (1)
- CHANGELOG.md
✅ Files skipped from review due to trivial changes (1)
- src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/data_processing/test_assembler.py
- src/nemo_safe_synthesizer/telemetry.py
- tests/training/test_huggingface_backend.py
- docs/user-guide/configuration.md
- tests/generation/test_processors.py
- tests/smoke/conftest.py
- src/nemo_safe_synthesizer/llm/metadata.py
- src/nemo_safe_synthesizer/config/training.py
- src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
- src/nemo_safe_synthesizer/training/huggingface_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Smoke Tests
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
🧰 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/preflight/checks/environment.pytests/llm/test_utils.pydocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/llm/utils.pytests/llm/test_metadata.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/preflight/checks/environment.pytests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.pytests/llm/test_metadata.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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/llm/utils.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/preflight/checks/environment.pysrc/nemo_safe_synthesizer/llm/utils.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/preflight/checks/environment.pytests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.pytests/llm/test_metadata.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/preflight/checks/environment.pytests/llm/test_utils.pypyproject.tomldocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/llm/utils.pytests/llm/test_metadata.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/preflight/checks/environment.pytests/llm/test_utils.pypyproject.tomldocs/user-guide/troubleshooting.mdsrc/nemo_safe_synthesizer/llm/utils.pytests/llm/test_metadata.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Python API documentation should use Google-style docstrings that will be auto-generated into API reference pages via the mkdocstrings plugin
Files:
src/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/llm/utils.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Tests should mirror the
src/directory structure intests/
Files:
tests/llm/test_utils.pytests/llm/test_metadata.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.All Python test files must exist under the
tests/directory and be discoverable by pytest with naming following the conventiontest_*.pyor*_test.py
Files:
tests/llm/test_utils.pytests/llm/test_metadata.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/llm/test_utils.pytests/llm/test_metadata.py
**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
unit_testmarker is deprecated; useunitinstead
Files:
tests/llm/test_utils.pytests/llm/test_metadata.py
pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Package metadata, dependencies, and extras (cpu/cu128/engine) should be configured in
pyproject.tomlOrder sections in
pyproject.tomlas:[project],[dependency-groups],[project.optional-dependencies],[tool.uv],[build-system],[tool.*].The
pyproject.tomlfile must be kept in sync withuv.lockto avoid lock file drift. Runmake lock-checkto verify synchronization
Files:
pyproject.toml
⚙️ CodeRabbit configuration file
Treat pyproject.toml as high-risk. Check package metadata, uv indexes, dependency groups, optional extras, Python version bounds, hatch config, ty config, script entry points, dependency consistency, and whether changes require regenerating uv.lock.
Files:
pyproject.toml
**/*.toml
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.toml: Use spaces around=for key-value pairs in TOML files.
Use# commentformat for comments in TOML files; use inline comments for dependency pins.
Files:
pyproject.toml
**/*.{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.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.Markdown documentation files must pass ruff formatting and linting checks
Files:
docs/user-guide/troubleshooting.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.Documentation files should follow the Diataxis framework with content organized into directories:
getting-started/for tutorials,user-guide/for how-tos and reference,architecture/for explanations,reference/for API documentation, anddev-notes/for release notes and design posts
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
Files:
docs/user-guide/troubleshooting.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.md
🪛 LanguageTool
docs/user-guide/troubleshooting.md
[style] ~75-~75: Consider using “incompatible” to avoid wordiness.
Context: ...des several early 5.x releases that are not compatible with its runtime. Keep vLLM's exclusion...
(NOT_ABLE_PREMIUM)
🪛 Ruff (0.15.13)
src/nemo_safe_synthesizer/llm/utils.py
[warning] 590-590: Do not catch blind exception: Exception
(BLE001)
tests/llm/test_metadata.py
[error] 394-394: Possible hardcoded password assigned to: "bos_token"
(S105)
[error] 396-396: Possible hardcoded password assigned to: "eos_token"
(S105)
[error] 414-414: Possible hardcoded password assigned to: "bos_token"
(S105)
[error] 416-416: Possible hardcoded password assigned to: "eos_token"
(S105)
🔇 Additional comments (5)
src/nemo_safe_synthesizer/preflight/checks/environment.py (1)
209-215: LGTM!docs/user-guide/troubleshooting.md (1)
69-107: LGTM!Also applies to: 140-148
pyproject.toml (1)
28-28: LGTM!Also applies to: 92-92, 120-121, 126-126, 136-136, 143-144, 153-153, 160-160
tests/llm/test_metadata.py (1)
390-398: LGTM!Also applies to: 410-418, 735-746, 757-768, 803-825, 842-870, 872-898, 904-925, 933-952
tests/llm/test_utils.py (1)
7-33: LGTM!
ed7e966 to
c438737
Compare
b9d1a46 to
e2996a6
Compare
| class NSSTrainingAndGenerationEvent(BaseModel): | ||
| _event_name: ClassVar[str] = "train_and_generation_event" | ||
| _schema_version: ClassVar[str] = "1.4" | ||
| _schema_version: ClassVar[str] = "1.7" |
There was a problem hiding this comment.
Please put this in it's own PR. I promise I will approve it quickly :)
5d9542a
| "faker", | ||
| "datasets>=4.8.4", | ||
| "huggingface-hub>=0.34.4,<1", | ||
| "huggingface-hub>=1.3.0,<2", |
There was a problem hiding this comment.
huh, never realized it was duplicated here.
mckornfield
left a comment
There was a problem hiding this comment.
I have one run from slurm of this, not the latest commit, want me to do another or you got something going?
| class NSSTrainingAndGenerationEvent(BaseModel): | ||
| _event_name: ClassVar[str] = "train_and_generation_event" | ||
| _schema_version: ClassVar[str] = "1.4" | ||
| _schema_version: ClassVar[str] = "1.7" |
There was a problem hiding this comment.
This change is still visible in the github console view I'm seeing.
Migrate from transformers 4.57.3 to v5 (>=5.0,<6). Fix the hard-removed v4 APIs so the existing import surface continues to work. Dependency floors (v5 requires these): - transformers>=5.0,<6 in cpu + cu129 extras - accelerate>=1.1.0, peft>=0.18.0, bitsandbytes>=0.46.1 - huggingface-hub>=1.3,<2 vllm interaction: keep the transformers>=5.0,<6 entry in override-dependencies because vllm pins transformers<5 in published metadata; the override is what lets uv resolve v5 against vllm 0.20.0. Revisit once vllm publishes a v5-aware build. Code changes: - dp_utils.py: drop the `utils.is_safetensors_available()` guard around `import safetensors.torch`. v5 makes safetensors a hard dep so the helper was removed; the import is now unconditional. - dp_utils.py: remove the `safe_serialization=` kwarg from PreTrainedModel.save_pretrained() calls (parameter dropped in v5, safetensors is the only supported format). - llm/metadata.py: import torch and call model_rebuild() eagerly on the ModelMetadata subclasses. Pydantic 2.12 + v5 type hints (PretrainedConfig | None) + `from __future__ import annotations` leave forward refs unresolved on first instantiation otherwise. - huggingface_backend.py: `evaluation_strategy=` was already named `eval_strategy=` in the source; no change needed. `warmup_ratio` survived as a v5 kwarg (not renamed to `warmup_step` — the migration guide is ambiguous; the actual v5 TrainingArguments keeps both `warmup_steps` and `warmup_ratio`). uv.lock regenerated against the new vllm+torch+cuda stack from mschwab/chore/bump-vllm-torch-stack. Signed-off-by: mkornfield <mkornfield@nvidia.com> Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Drop the v4.48-era try/except shim around compute_loss in OpacusDPTrainer. The num_items_in_batch parameter is part of the stable Trainer.compute_loss signature in v5, so the TypeError fallback is unreachable now that the minimum supported version is transformers>=5.0. Tighten the surrounding comment to describe what the call does (skip HF's built-in per-token scaling so Opacus's per-sample scaling stays authoritative) instead of tracking which HF version introduced the kwarg. Signed-off-by: mkornfield <mkornfield@nvidia.com> Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
…nd fast-tokenizer helper Adds first-class support for transformers v5 quantization backends through a new `training.quantization_scheme` config field, and centralizes tokenizer loads to surface v5's Rust-backend fallback. Quantization (config/training.py + llm/utils.py + training/huggingface_backend.py): - New `QuantizationScheme` StrEnum: bnb-4bit, bnb-8bit, fp8, nvfp4, mxfp4 - New `training.quantization_scheme` field (pydantic). Falls back to the legacy `quantization_bits` when unset (4 → bnb-4bit, 8 → bnb-8bit) - `get_quantization_config()` is now scheme-dispatched and returns the right v5 config class: `BitsAndBytesConfig`, `FineGrainedFP8Config`, `TorchAoConfig(NVFP4WeightOnlyConfig)`, or `Mxfp4Config`. Integer callers (4/8) still work for backward compatibility - `_get_quantization_config_if_enabled()` resolves the scheme via a small `_resolve_quantization_scheme()` helper and logs `scheme=<value>` - LoftQ branch in `_prepare_quantize_base()` now raises `ParameterError` for non-bitsandbytes schemes (LoftQ requires the BNB runtime) and reads bit width from `scheme.effective_bits` - Preflight memory estimator (`bytes_per_base_weight()`) prefers the scheme's `effective_bits` and falls back to `quantization_bits` Tokenizer (llm/utils.py + llm/metadata.py + training/huggingface_backend.py): - New `load_fast_tokenizer()` helper that forwards `use_fast=True` and warns when v5 falls back to the slow Python backend. This surfaces a signal that was silent under v4's separate-file Rust/slow split - All four `AutoTokenizer.from_pretrained` call sites now go through it; unused `AutoTokenizer` imports removed Tests: - Update `TestGetQuantizationConfigIfEnabled` to assert against the `QuantizationScheme` enum the resolver passes - New test: `test_explicit_scheme_overrides_bits` for the new precedence - Fix `test_load_pretrained_model_uses_cached_model_ref_target_for_tokenizer` to patch `load_fast_tokenizer` instead of `AutoTokenizer.from_pretrained` Docs: - `docs/user-guide/configuration.md`: new Quantization schemes section with per-scheme hardware/bits/backend table and LoftQ compatibility note - `docs/user-guide/troubleshooting.md`: two new installation entries — the vLLM-transformers-v5 resolution conflict and the slow-tokenizer warning users will see on models without a Rust port - `CHANGELOG.md`: New Features + Improvements entries Signed-off-by: mkornfield <mkornfield@nvidia.com> Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: mkornfield <mkornfield@nvidia.com> Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Adds a train-only smoke test so the explicit v5 quantization_scheme API does not bitrot; runs under make test-smoke-gpu-train-only. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Remove evaluation-only type suppress that was unrelated to the v5 upgrade. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Keep the legacy alias for back-compat but surface deprecation in the Field schema per review feedback. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Drop the misleading "model is too new" example; describe missing tokenizer.json on older SentencePiece checkpoints instead. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Wrap dependency pins in a quoted [project].dependencies array so the troubleshooting snippet passes TOML highlighting. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
5d9542a to
bc2a1ae
Compare
|
@kendrickb-nvidia - rebased it and the telemetry changes render correctly now (and by correctly, i mean they are no longer visible). |
followup from #483 - there's a regression in memory performance in transformers v5. this is the minimal fix for our specific workloads; I'll put up a more comprehensive change that should help with observability and memory pressure during training after this. ## Summary This PR lowers the physical microbatch size used by the DP run configs from `8` to `4` and raises `gradient_accumulation_steps` to `16`. The effective batch size stays at `64`, but each forward/backward pass uses fewer examples. This is the minimal fix for the recent larger-model DP OOMs without changing the shared training defaults for non-DP runs. ## Why DP training is not inherently constrained to `batch_size: 1`, but larger physical batches increase activation and per-sample-gradient memory. The prior DP configs used `batch_size: 8` with the global default accumulation of `8`. Moving to `4 x 16` preserves optimizer-step batch semantics while reducing peak memory pressure. ## Changes - Update DP SLURM configs to use: - `training.batch_size: 4` - `training.gradient_accumulation_steps: 16` - Update required e2e DP configs to match the new DP run shape. ## Test Plan - `make check` - slurm run (will report back) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Reduced per-device training batch sizes across distributed-parallel configs (mostly 8→4; one model adjusted to 2). * Standardized/added gradient accumulation to 16 across DP training configs to preserve effective batch size. * Result: improved memory efficiency and more balanced resource use for distributed training runs. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA-NeMo/Safe-Synthesizer/pull/528?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com> Signed-off-by: Matthew Kornfield <mkornfield@nvidia.com> Co-authored-by: Matthew Kornfield <mkornfield@nvidia.com>
Summary
Stacked on top of #464 (vLLM 0.20 + torch 2.11 + CUDA 12.9 bump). When that lands, this PR auto-rebases onto
main.accelerate>=1.1.0,peft>=0.18.0,bitsandbytes>=0.46.1,huggingface-hub>=1.3,<2.utils.is_safetensors_available()(helper removed in v5; safetensors is mandatory), dropssafe_serialization=fromsave_pretrained()(parameter gone), and eagerly callsmodel_rebuild()onModelMetadatasubclasses to resolve Pydantic 2.12 + v5 type hints withfrom __future__ import annotations.training.quantization_schemeconfig field:bnb-4bit,bnb-8bit,fp8,nvfp4,mxfp4. Legacyquantization_bitsstill works (back-compat).get_quantization_config()now dispatches on the scheme and returns the right v5 config class (BitsAndBytesConfig,FineGrainedFP8Config,TorchAoConfig(NVFP4WeightOnlyConfig),Mxfp4Config).ParameterErrorwhen paired with a non-bitsandbytes scheme (LoftQ requires the BNB runtime).scheme.effective_bitsfor non-BNB schemes.load_fast_tokenizer()helper centralizes the 4AutoTokenizer.from_pretrainedcall sites and logs a warning when v5 falls back to the slow Python backend.configuration.md(per-scheme hardware/bits/backend table), troubleshooting entries for the vLLM-v5 resolver conflict and the slow-tokenizer warning, CHANGELOG entries.Backward compatibility
quantization_bits: 4orquantization_bits: 8and noquantization_schemecontinue to work — the resolver maps them tobnb-4bit/bnb-8bitand theget_quantization_config(4)/get_quantization_config(8)integer callable signature is preserved.Trainer(processing_class=...)call site was already v5-correct; no behavioral change.is_safetensors_available()removal makessafetensors.torchan unconditional import — v5 already requires this.Test plan
make testandmake test-e2e