feat: add DP memory controls and physical batch sizing#518
feat: add DP memory controls and physical batch sizing#518binaryaaron wants to merge 16 commits into
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:
WalkthroughThe PR introduces Opacus per-sample gradient mode selection ( ChangesDP training memory control, batching resolution, and observability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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! |
9459c70 to
aee3ee6
Compare
28b0729 to
7967224
Compare
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: ccb2f659-e86a-4d5d-acd2-485856ffab2e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (22)
docs/user-guide/configuration.mddocs/user-guide/troubleshooting.mdpyproject.tomlsrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/config/differential_privacy.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/training/training_observability.pytests/cli/test_run.pytests/config/test_parameters.pytests/configurator/test_pydantic_click_options.pytests/observability/test_observability.pytests/preflight/test_preflight.pytests/sdk/test_builder.pytests/smoke/test_training_cpu.pytests/training/test_huggingface_backend.pytests/training/test_training_observability.py
📜 Review details
🧰 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:
src/nemo_safe_synthesizer/training/training_observability.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.pydocs/user-guide/troubleshooting.mdtests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/config/differential_privacy.pytests/observability/test_observability.pytests/training/test_training_observability.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pydocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/smoke/test_training_cpu.pysrc/nemo_safe_synthesizer/config/training.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/config/test_parameters.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported
**/*.py: UseNSSBaseModelfor config/parameter models inconfig/which define the user-facing configuration of NSS. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields for both Python name and env var name mapping.
Always includeField(description=...)for Pydantic model fields as the canonical field docstring
Use assignment-styleField()for Pydantic model fields (e.g.,field_name = Field(default=..., description=...)) instead ofAnnotatedwhen default, default_factory, or alias are used
UseAnnotatedonly when the field carries additional metadata beyondField()such as validators,AutoParam,DependsOnValidator, or constrained type aliases
For mutable defaults in dataclasses, usefield(default_factory=list)never= []
UseStrEnumfor string-valued enums used in configs/serialization. Use plainEnumfor internal-only named constants.
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 logger calls for data that downstream tools should query or aggregate (metrics, counts, durations). Use f-strings for human-readable context that doesn't need machine parsing.
Raise errors from the custom hierarchy with dual inheritance:SafeSynthesizerError,UserError,DataError,ParameterError,GenerationError,InternalError
Target Python 3.11–3.13; do not us...
Files:
src/nemo_safe_synthesizer/training/training_observability.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/config/differential_privacy.pytests/observability/test_observability.pytests/training/test_training_observability.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/smoke/test_training_cpu.pysrc/nemo_safe_synthesizer/config/training.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/config/test_parameters.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/nemo_safe_synthesizer/**/*.py: Source code must remain Python 3.11 syntax-compatible. Do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Write Python docstrings in Google style format so they auto-generate into API reference pages via mkdocstrings and gen-files plugins
Files:
src/nemo_safe_synthesizer/training/training_observability.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/differential_privacy.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) must include SPDX copyright headers. Use mise run format to add them automatically
Files:
src/nemo_safe_synthesizer/training/training_observability.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.pydocs/user-guide/troubleshooting.mdtests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/config/differential_privacy.pytests/observability/test_observability.pytests/training/test_training_observability.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pydocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/smoke/test_training_cpu.pysrc/nemo_safe_synthesizer/config/training.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/config/test_parameters.py
**/*
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
src/nemo_safe_synthesizer/training/training_observability.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.pydocs/user-guide/troubleshooting.mdtests/preflight/test_preflight.pypyproject.tomlsrc/nemo_safe_synthesizer/config/differential_privacy.pytests/observability/test_observability.pytests/training/test_training_observability.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pydocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/smoke/test_training_cpu.pysrc/nemo_safe_synthesizer/config/training.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/config/test_parameters.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/training_observability.pysrc/nemo_safe_synthesizer/training/huggingface_backend.py
src/**/*.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/training/training_observability.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/config/differential_privacy.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pysrc/nemo_safe_synthesizer/preflight/checks/environment.pysrc/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
src/nemo_safe_synthesizer/training/training_observability.pytests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pysrc/nemo_safe_synthesizer/sdk/config_builder.pysrc/nemo_safe_synthesizer/cli/wandb_setup.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/training/test_huggingface_backend.pydocs/user-guide/troubleshooting.mdtests/preflight/test_preflight.pypyproject.tomlsrc/nemo_safe_synthesizer/config/differential_privacy.pytests/observability/test_observability.pytests/training/test_training_observability.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/observability.pydocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/preflight/checks/environment.pytests/smoke/test_training_cpu.pysrc/nemo_safe_synthesizer/config/training.pytests/cli/test_run.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.pytests/config/test_parameters.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Define pytest markers inpytest.iniwith--strict-markersenabled. Use exactly one category marker (unit,smoke,e2e) per test. Category modifiers includeslowfor long-running tests,requires_gpufor CUDA-dependent tests,vllmfor vLLM backend tests,smollm2for SmolLM2 Hub download tests, andnoautouseto skip autouse fixtures
ForParsedResponsemocking, usevalid_records=[...],invalid_records=[...],errors=[...], andprompt_number=int. Usefixture_mock_processororfixture_mock_processor_without_valid_recordshelpers
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformersandvllmrequirecu129extra)
For vLLM tests that call.generate(), mark with@pytest.mark.vllm, use per-file process isolation (-n 0), and create dedicatedtest:smoke:gpu:*mise tasks. vLLM pre-allocates all GPU memory and never releases it within a process, causing OOM in later tests if not isolated
Convert nullable columns topd.Int64Dtype()orpd.BooleanDtype()before assigningnp.nanto avoid pandas dtype conversion errors
When using Faker for test data generation, callfake.seed_instance(seed)andrandom.seed(seed)for reproducibility
Useprint()statements in test functions for debug output (ruffT201is suppressed fortests/). Import shared methods fromconftest.pyusing relative imports when needed across test files (e.g.,from .conftest import train_with_sdk). Do not import from other test files liketests/cli/helpers.pydue to pytest discovery limitations
tests/**/*.py: File naming for tests:test_*.py; class naming:Test*; function naming:test_<module>_<expected_behavior>
Usefixture_prefix convention for pytest fixtures for grep-ability; add a one-line docstring describing the fixture's purpose and data
Use function-scoped fixtures by default in pytest; session scope only when empirically justified by test runtime
Use...
Files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning Tests
All mise test tasks, grouped by scope:
mise run test # Unit (excludes slow, e2e, and smoke) mise run test:unit-slow # Unit tests including slow (excludes e2e and smoke) mise run test:smoke # CPU smoke tests (~few min, no GPU required) mise run test:smoke:gpu # All staged GPU smoke tests (requires CUDA) mise run test:smoke:gpu:train-only mise run test:smoke:gpu:generation mise run test:smoke:gpu:resume mise run test:smoke:gpu:structured-generation mise run test:smoke:gpu:timeseries mise run test:smoke:gpu:smollm2 mise run test:e2e # All e2e (requires CUDA) -- runs default + dp mise run test:e2e:default # e2e default (no-DP) tests only mise run test:e2e:dp # e2e DP tests only mise run test:ci # CI unit tests with coverage (excludes slow, e2e, gpu, smoke) mise run test:ci-slow # CI slow tests with coverage mise run test:ci-container # CI tests in a Linux container (Docker/Podman)Run a single test:
uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/configuration.md
docs/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/writing-docs.mdc)
docs/**/*.md: Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for highlighting important information and collapsible sections in documentation
Use MkDocs Material tabs syntax (=== "Label") to present alternative views or language-specific examples in documentation
Use code block syntax with title and highlight line parameters (title="filename", hl_lines="2 3") for code examples in documentation
Use Mermaid diagram syntax (```mermaid flowchart, etc.) for visualizations in documentationClassify documentation pages in
docs/as tutorial, how-to, explanation, or reference per the Diataxis framework. Use MkDocs Material syntax: admonitions (!!! note), tabs (===), code blocks with titles and highlights.Documentation markdown files must follow MkDocs Material Markdown extensions syntax including admonitions, content tabs, code blocks with annotations, and Mermaid diagrams
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/configuration.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.md: No decorative**bold**in markdown body text, list items, or docstrings. Use headers, list markers, colons, and backticks for structure.
Use--(em-dash) for asides in markdown, not-(hyphen)
Use single backticks for code identifiers, paths, and CLI commands in markdown
For Mermaid diagrams: use no spaces in node IDs, quote labels with special characters, avoid explicit colors or stylesMarkdown documentation must be checked for consistency and formatting. Run mise run check to validate markdown files
Files:
docs/user-guide/troubleshooting.mddocs/user-guide/configuration.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/configuration.md
pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Configure package metadata, dependencies, extras (cpu/cu129/engine), and uv configuration 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# commentin TOML files; use inline comments for dependency pins
Files:
pyproject.toml
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/differential_privacy.pysrc/nemo_safe_synthesizer/config/parameters.pysrc/nemo_safe_synthesizer/config/training.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
🧠 Learnings (11)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/configurator/test_pydantic_click_options.pytests/sdk/test_builder.pytests/training/test_huggingface_backend.pytests/preflight/test_preflight.pytests/observability/test_observability.pytests/training/test_training_observability.pytests/smoke/test_training_cpu.pytests/cli/test_run.pytests/config/test_parameters.py
📚 Learning: 2026-06-03T23:08:50.142Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: tests/TESTING.md:0-0
Timestamp: 2026-06-03T23:08:50.142Z
Learning: Applies to tests/**/*.py : For `ParsedResponse` mocking, use `valid_records=[...]`, `invalid_records=[...]`, `errors=[...]`, and `prompt_number=int`. Use `fixture_mock_processor` or `fixture_mock_processor_without_valid_records` helpers
Applied to files:
tests/sdk/test_builder.py
📚 Learning: 2026-06-05T23:48:32.983Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: STYLE_GUIDE.md:0-0
Timestamp: 2026-06-05T23:48:32.983Z
Learning: Applies to **/*.py : Use `NSSBaseModel` for config/parameter models in `config/` which define the user-facing configuration of NSS. Use raw `BaseModel` or module-specific bases for data transfer objects and internal structures.
Applied to files:
src/nemo_safe_synthesizer/sdk/config_builder.py
📚 Learning: 2026-06-05T23:48:32.983Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: STYLE_GUIDE.md:0-0
Timestamp: 2026-06-05T23:48:32.983Z
Learning: Applies to **/*.py : Use `Protocol` for structural subtyping when you need duck-typing boundaries in Python
Applied to files:
src/nemo_safe_synthesizer/cli/wandb_setup.py
📚 Learning: 2026-06-03T23:09:02.641Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: .cursor/rules/repo-navigation.mdc:0-0
Timestamp: 2026-06-03T23:09:02.641Z
Learning: Applies to pyproject.toml : Configure package metadata, dependencies, extras (cpu/cu129/engine), and uv configuration in `pyproject.toml`
Applied to files:
pyproject.toml
📚 Learning: 2026-06-05T23:48:32.983Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: STYLE_GUIDE.md:0-0
Timestamp: 2026-06-05T23:48:32.983Z
Learning: Applies to **/*.py : Use `Annotated` only when the field carries additional metadata beyond `Field()` such as validators, `AutoParam`, `DependsOnValidator`, or constrained type aliases
Applied to files:
src/nemo_safe_synthesizer/config/differential_privacy.py
📚 Learning: 2026-06-03T23:08:50.142Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: tests/TESTING.md:0-0
Timestamp: 2026-06-03T23:08:50.142Z
Learning: Applies to tests/**/*.py : Use `print()` statements in test functions for debug output (ruff `T201` is suppressed for `tests/`). Import shared methods from `conftest.py` using relative imports when needed across test files (e.g., `from .conftest import train_with_sdk`). Do not import from other test files like `tests/cli/helpers.py` due to pytest discovery limitations
Applied to files:
tests/smoke/test_training_cpu.py
📚 Learning: 2026-06-03T23:08:50.142Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: tests/TESTING.md:0-0
Timestamp: 2026-06-03T23:08:50.142Z
Learning: Applies to tests/**/conftest.py : Define test data fixtures with the `fixture_` prefix (e.g., `fixture_iris_dataset`, `fixture_dow_jones_index_dataset`, `fixture_clinc_oos_dataset`). Store stub datasets in `tests/stub_datasets/` and use helper functions `load_test_dataset(filename)` and `load_test_dataframe(filename)` from root `conftest.py` to load test data
Applied to files:
tests/smoke/test_training_cpu.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/smoke/test_training_cpu.py
📚 Learning: 2026-06-05T23:48:32.983Z
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 0
File: STYLE_GUIDE.md:0-0
Timestamp: 2026-06-05T23:48:32.983Z
Learning: Applies to **/*.py : Add `from __future__ import annotations` to every Python module to make all annotations strings consistently
Applied to files:
src/nemo_safe_synthesizer/config/training.pysrc/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
📚 Learning: 2026-06-01T18:13:50.774Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 538
File: src/nemo_safe_synthesizer/cli/utils.py:0-0
Timestamp: 2026-06-01T18:13:50.774Z
Learning: In `src/nemo_safe_synthesizer/cli/utils.py`, `_propagate_runtime_settings_to_env` intentionally uses write-only (non-`None`) propagation. The function runs exactly once per `common_setup` call in a one-shot CLI process, so there is no prior-invocation leakage risk. A clear-on-`None` guard (else: os.environ.pop(...)) would also be ineffective because `CLISettings` re-reads `os.environ` at construction time, meaning any value written by a hypothetical prior run is already reflected as non-`None` in the field — the else branch would never fire. A real cross-run isolation fix would require snapshotting/restoring `os.environ` around each run, which is out of scope for this PR.
Applied to files:
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
🪛 Ruff (0.15.15)
tests/smoke/test_training_cpu.py
[warning] 182-182: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py
[warning] 215-215: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
[warning] 241-241: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
[error] 891-892: try-except-pass detected, consider logging the exception
(S110)
🔇 Additional comments (18)
src/nemo_safe_synthesizer/preflight/checks/environment.py (1)
194-229: LGTM!Also applies to: 392-400
tests/preflight/test_preflight.py (1)
260-282: LGTM!docs/user-guide/configuration.md (1)
71-77: LGTM!Also applies to: 222-223
docs/user-guide/troubleshooting.md (1)
157-179: LGTM!Also applies to: 439-465
src/nemo_safe_synthesizer/config/training.py (1)
7-7: LGTM!Also applies to: 35-40, 157-171, 174-230, 266-303, 501-538
src/nemo_safe_synthesizer/config/differential_privacy.py (1)
8-9: LGTM!Also applies to: 78-90
src/nemo_safe_synthesizer/config/parameters.py (1)
217-221: LGTM!src/nemo_safe_synthesizer/sdk/config_builder.py (1)
9-9: LGTM!Also applies to: 122-126
tests/config/test_parameters.py (1)
12-37: LGTM!Also applies to: 46-47, 50-211
tests/cli/test_run.py (1)
844-847: LGTM!Also applies to: 902-905, 937-957
tests/configurator/test_pydantic_click_options.py (1)
483-512: LGTM!tests/sdk/test_builder.py (1)
9-12: LGTM!Also applies to: 197-208
src/nemo_safe_synthesizer/cli/wandb_setup.py (2)
278-278: Already flagged: remove the Ellipsis default from the Protocol method parameter.
14-15: LGTM!Also applies to: 268-277, 281-302
src/nemo_safe_synthesizer/training/training_observability.py (1)
1-84: LGTM!src/nemo_safe_synthesizer/observability.py (1)
84-85: LGTM!Also applies to: 212-216, 1016-1149
tests/training/test_training_observability.py (1)
1-135: LGTM!tests/observability/test_observability.py (1)
298-314: LGTM!
Greptile SummaryThis PR adds OOM-mitigation controls and structured observability to the DP training path for the transformers v5 upgrade. It replaces ad-hoc
Confidence Score: 5/5Safe to merge; training correctness and DP accounting are preserved across both gradient modes. The DP accounting invariant (effective batch = physical batch × accumulation steps) is correctly preserved by resolve_batching(), and both the hooks and ghost clipping paths are covered by CPU smoke tests. The two findings are observability-only: the peak_loss_logits_gb_bucket_le metric over-reports when chunking is active, and the opacus version spec is looser than stated in the PR description. Neither affects training correctness or DP guarantees. src/nemo_safe_synthesizer/privacy/dp_transformers/dp_utils.py — the _record_peak_loss_logits call and opacus version spec in pyproject.toml are worth a second look before the next uv lock upgrade. Important Files Changed
|
|
Let's land the vllm observability pr first. |
7e846ca to
e750a59
Compare
02367e5 to
b03c44b
Compare
b03c44b to
7843ee4
Compare
mckornfield
left a comment
There was a problem hiding this comment.
just bumped some of andre's comments, but lg for my old ones
| updated_params = merge_dicts(self._config.model_dump(exclude_unset=True), new_params) | ||
| logger.debug(f"params to update: {updated_params}") | ||
| my_config = SafeSynthesizerParameters.model_validate(updated_params) | ||
| logger.debug(f"params to update: {new_params}") |
There was a problem hiding this comment.
curious, did we have an effort to do structured logging/ is that applicable?
There was a problem hiding this comment.
agent (
review-pr): Structured logging is applicable here
get_logger(__name__) already routes these messages through the structured logging stack, but the dictionaries are currently interpolated into the event text rather than emitted as structured context. Using extra={"ctx": ...} would make the fields queryable. This is a valid cleanup; it does not affect the configuration behavior changed here.
|
|
||
|
|
||
| def _nested_parameters_type(annotation: object) -> type[Parameters] | None: | ||
| from .parameters import Parameters |
There was a problem hiding this comment.
super nit, should we lift these guys up, since this is also in the type check? or is it here because it's expensive?
There was a problem hiding this comment.
agent (
review-pr): The local import avoids a module cycle
This placement is intentional. configurator.parameters imports ParameterSchema from parameter_paths at module scope, so importing Parameters back at the top of parameter_paths would create a circular import. TYPE_CHECKING supplies the annotation, and the local runtime import defers the reverse dependency until the helper is called.
There was a problem hiding this comment.
can we address this guy?
| model = GradSampleModule(model) | ||
| if grad_sample_mode == "hooks": | ||
| model = GradSampleModule(model) | ||
| elif grad_sample_mode == "ghost": |
f317956 to
8f1d5d6
Compare
| valid_positions = torch.nonzero((shift_labels != -100).any(dim=0), as_tuple=False).flatten() | ||
| if valid_positions.numel() == 0: | ||
| raise RuntimeError("Ghost clipping DP training requires at least one non-ignored label.") | ||
| model_inputs.setdefault("logits_to_keep", valid_positions.to(labels.device)) | ||
|
|
||
| forward_model = self.accelerator.unwrap_model(model, keep_fp32_wrapper=False) | ||
| outputs = forward_model(**model_inputs) | ||
| logits = getattr(outputs, "logits", None) | ||
| if logits is None and isinstance(outputs, dict): | ||
| logits = outputs.get("logits") | ||
| if logits is None: | ||
| raise RuntimeError("Ghost clipping DP training requires model outputs with logits.") | ||
| if logits.ndim != 3 or logits.shape[1] != valid_positions.numel(): | ||
| raise RuntimeError( | ||
| "Ghost clipping DP training expected the model to honor logits_to_keep " | ||
| f"with {valid_positions.numel()} kept positions, got logits shape {tuple(logits.shape)}." | ||
| ) | ||
|
|
||
| shift_logits = logits.contiguous() | ||
| shift_labels = shift_labels.index_select(1, valid_positions.to(shift_labels.device)).to(shift_logits.device) | ||
| return self.dp_loss( | ||
| shift_logits.view(-1, shift_logits.shape[-1]), | ||
| shift_labels.view(-1), | ||
| shape=shift_logits.shape, |
There was a problem hiding this comment.
Ghost mode keeps a token column when any sequence in the batch has a valid label there. In a mixed-length batch, a shorter sequence can still have -100 at one of those kept columns. That ignored position is passed into the Opacus loss reduction as a zero loss, so it is counted in the per-sequence mean instead of being excluded. The hooks path divides each sequence by its own valid-token count, but this path can shrink the loss and gradients for shorter examples, changing the clipping behavior for the same DP unit. This is reachable with batches where examples have different label masks.
Context Used: AGENTS.md (source)
Add ghost clipping support and diagnostic loss controls so DP training can reduce and inspect memory pressure on larger models. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Use the model quantization flag rather than PEFT mode to estimate base-weight memory so preflight warnings match actual loading behavior. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Resolve runtime Trainer batch arguments from a physical microbatch cap while preserving the configured effective batch for training and DP accounting. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Preserve validated privacy and batching config paths while fixing ghost clipping adapter saves and generation progress rate rendering. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Tighten the updated tests around resolver contracts, DP adapter saves, and SDK validation so they assert behavior rather than implementation details. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Emit a schema-frozen TrainingObservability event (peak VRAM via NvmlPeakSampler, peak loss-logits, resolved batching, grad-sample mode) once per fit, routed to structured logs and mirrored to wandb through a generic log_observability_event sink. Promote the shared NVML/loadavg sampling primitives into observability.py. Replace the env-var memory knobs with a first-class TrainingMemoryControls config nested under training.memory, plumbed into OpacusDPTrainer. The opt-in causal-LM loss probe now installs and tears down symmetrically in train(), and the trainer stashes the event for the backend to forward so the privacy layer stays independent of the CLI. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
When nvmlInit() succeeds but nvmlDeviceGetHandleByIndex() raises, balance the init with nvmlShutdown() before degrading to a no-op sampler; otherwise NVML stayed initialized while _pynvml was nulled, leaking the init refcount. Keeps NvmlPeakSampler byte-identical with the generation-side observability branch. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Validate flat config overrides consistently and preserve DP training observability on failure while guarding the loss probe and ghost-clipping paths. Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
8f1d5d6 to
0535fcb
Compare
Summary
Adds OOM-mitigation controls and observability to the DP training path for the transformers v5 upgrade.
training.memory(TrainingMemoryControls) replaces the oldNSS_*env-var knobs with first-class, validated fields --disable_dp_bf16,chunked_causal_lm_loss,chunked_causal_lm_loss_tokens,debug_loss_memory. Plumbed intoOpacusDPTrainer.training.max_physical_batch_sizecaps the per-device microbatch andgradient_accumulation_steps="auto"derives accumulation from the logical batch.resolve_batching()returns a singleResolvedTrainingBatchingthe backend consumes; DP guarantees hold across accumulation steps.differential_privacy.grad_sample_mode(hooks|ghost) selecting Opacus fast vs ghost gradient clipping;opacuspinned to==1.6.0.quantize_modeluses unquantized VRAM) instead of assuming quantized weights.TrainingObservabilityevent (peak VRAM viaNvmlPeakSampler, peak loss-logits, resolved batching, grad-sample mode) is emitted once per fit to structured logs and mirrored to wandb through a genericlog_observability_eventsink (WandbLoggableProtocol). Shared NVML/loadavg sampling primitives now live inobservability.py. The opt-in causal-LM loss probe installs/tears down symmetrically intrain(); the trainer stashes the event for the backend to forward, keeping the privacy layer independent of the CLI.Coordination
The generic
log_observability_eventsink and the promoted NVML primitives are intended to converge with the generation-side observability work (#526). #526 lands first as the canonical home; this branch rebases and drops duplicates afterward.Test plan
mise run check(ruff + format + copyright + ty)TrainingMemoryControls, batching), configurator CLI flags, preflight VRAM, observability event/sinkSummary by CodeRabbit
Release Notes
New Features
"hooks"and"ghost".training.completeanalytics event including resolved batching and DP details.Documentation
training.quantization_scheme.Bug Fixes
Chores
1.6.0.