fix(llm): size VRAM headroom from reclaimable memory on integrated GPUs#674
fix(llm): size VRAM headroom from reclaimable memory on integrated GPUs#674marcusds wants to merge 2 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:
WalkthroughChangesThe VRAM helpers now account for Linux Integrated GPU memory handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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: da1a70ab-c4fa-4444-9e2d-63d380bff6d7
📒 Files selected for processing (2)
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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/llm/utils.pytests/llm/test_utils.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: Use American English spelling in Python code, documentation, and messages.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly for additional metadata such as validators, constrained aliases, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports for package code undersrc/.
Do not useassertfor validation in library code; raise an appropriate exception instead.Write Google-style docstrings for public Python APIs because API reference pages are generated from source docstrings.
Files:
src/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/llm/utils.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
**/*: All contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_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:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/llm/test_utils.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/llm/test_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/llm/test_utils.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/llm/test_utils.py
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/llm/test_utils.py
🔇 Additional comments (2)
src/nemo_safe_synthesizer/llm/utils.py (1)
439-454: LGTM!Also applies to: 457-488
tests/llm/test_utils.py (1)
7-13: LGTM!Also applies to: 77-86, 105-127
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a false-negative preflight VRAM check on integrated GPUs (e.g. DGX Spark / GB10) where
Confidence Score: 5/5Safe to merge for bare-metal integrated-GPU hosts; one note about cgroup-limited container environments worth considering before wide platform rollout. The fix is correct and well-scoped: discrete-GPU behavior is unchanged, the reclaimable value is read once before the loop, the min(max(free, reclaimable), total) expression is mathematically sound, and the tests cover the main paths including the cap and the fallback. The only open question is cgroup-limited containers, which is a deployment-time concern that doesn't affect bare-metal or full-memory-access container scenarios. No files require special attention; the cgroup note in utils.py is worth revisiting if this path runs inside containers with per-container memory limits on the NeMo Platform. Important Files Changed
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemo_safe_synthesizer/llm/utils.py (1)
573-574: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the cgroup-bounded value rather than taking the larger value.
When CUDA reports 16 GiB free but cgroup-bounded reclaimable memory is 1 GiB,
max()retains 16 GiB and bypasses the new container limit. This can exceed the container allowance on integrated GPUs. Replace this withfree = min(reclaimable, total); the existingreclaimable is Nonebranch already preserves CUDA fallback.Proposed fix
- free = min(max(free, reclaimable), total) + free = min(reclaimable, total)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 91e78d2b-1690-47d8-9655-c869128dcf02
📒 Files selected for processing (2)
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.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: Use American English spelling in Python code, documentation, and messages.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly for additional metadata such as validators, constrained aliases, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
tests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/llm/test_utils.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/llm/test_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/llm/test_utils.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/llm/test_utils.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
**/*: All contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
tests/llm/test_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/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
tests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
tests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
tests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
tests/llm/test_utils.pysrc/nemo_safe_synthesizer/llm/utils.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports for package code undersrc/.
Do not useassertfor validation in library code; raise an appropriate exception instead.Write Google-style docstrings for public Python APIs because API reference pages are generated from source docstrings.
Files:
src/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/llm/utils.py
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/llm/test_utils.py
🪛 ast-grep (0.44.1)
src/nemo_safe_synthesizer/llm/utils.py
[warning] 464-464: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🔇 Additional comments (3)
tests/llm/test_utils.py (3)
13-13: LGTM!
100-100: LGTM!Also applies to: 117-138
165-225: LGTM!
98bd6d5 to
0448df1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nemo_safe_synthesizer/llm/utils.py (1)
471-497: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnreadable/unparsable cgroup usage silently treated as zero, overestimating headroom.
At lines 490-494, when
raw_usageisNone(unreadable) or failsint()parsing,usagedefaults to0, soheadroom = max(limit - usage, 0)becomes the fulllimit. In a memory-limited container where the usage file is transiently unreadable, this reports the entire limit as "remaining," which_get_vram_allocationsthen uses to size an allocation near the full limit — risking OOM. This is the same issue flagged in a prior review of this function ("Do not treat unreadable cgroup usage as zero ... Propagate an unavailable cgroup-data state").Skip the depth (mirroring the existing
raw_limit is None or raw_limit == "max"pattern) instead of assuming zero usage, so an unknown usage doesn't silently produce an overly generous headroom estimate.🛡️ Proposed fix
raw_usage = _read_cgroup_file(f"{directory}/{usage_file}") - try: - usage = int(raw_usage) if raw_usage is not None else 0 - except ValueError: - usage = 0 + if raw_usage is None: + continue + try: + usage = int(raw_usage) + except ValueError: + continue headroom = max(limit - usage, 0) remaining = headroom if remaining is None else min(remaining, headroom)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c6d74e54-23b3-441b-b35e-756e1acf880a
📒 Files selected for processing (2)
src/nemo_safe_synthesizer/llm/utils.pytests/llm/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (Python)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/llm/utils.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: Use American English spelling in Python code, documentation, and messages.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly for additional metadata such as validators, constrained aliases, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
src/nemo_safe_synthesizer/llm/utils.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports for package code undersrc/.
Do not useassertfor validation in library code; raise an appropriate exception instead.Write Google-style docstrings for public Python APIs because API reference pages are generated from source docstrings.
Files:
src/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/llm/utils.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
**/*: All contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
src/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:
src/nemo_safe_synthesizer/llm/utils.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
src/nemo_safe_synthesizer/llm/utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Shared Python package code must remain compatible with Python 3.11 syntax; do not use Python 3.12-only syntax such as PEP 695
typestatements or bracketed generic class/function parameters.
Files:
src/nemo_safe_synthesizer/llm/utils.py
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
src/nemo_safe_synthesizer/llm/utils.py
**/*.{py,sh}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's pinned
misetasks for formatting, linting, type checking, and testing rather than invokingruffortydirectly for project-wide checks.
Files:
src/nemo_safe_synthesizer/llm/utils.py
🪛 ast-grep (0.44.1)
src/nemo_safe_synthesizer/llm/utils.py
[warning] 464-464: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🔇 Additional comments (1)
src/nemo_safe_synthesizer/llm/utils.py (1)
439-454: LGTM!Also applies to: 457-468, 500-534, 537-585
8a1b299 to
e400df2
Compare
`_get_vram_allocations` derived available VRAM from `torch.cuda.mem_get_info().free`, which on integrated GPUs (e.g. NVIDIA GB10 / Grace, where GPU memory is system memory) counts only unallocated pages and ignores the reclaimable page cache. On such a device with most memory in buff/cache, `free` reads a couple GiB; after the 2 GiB safety buffer the usable fraction collapses to 0, and the `gpu.vram` preflight check hard-fails every job with "exceeds available ~0.0 GiB" even when tens of GiB are actually reclaimable. On integrated GPUs, use the kernel's `MemAvailable` (which accounts for reclaimable memory), capped at device total, instead of `mem_get_info`'s free figure. Discrete-GPU behavior is unchanged. Falls back to the CUDA free value when `/proc/meminfo` is unavailable (e.g. non-Linux hosts). Signed-off-by: mschwab <mschwab@nvidia.com>
Address PR review: read `MemAvailable` once above the per-GPU loop (it is a system-wide value) rather than per device, and add a regression test for the `min(free, total)` cap where `MemAvailable` exceeds device total, guarding the 2 GiB safety buffer. Signed-off-by: mschwab <mschwab@nvidia.com>
e400df2 to
9df2ba9
Compare
|
Thanks! makes sense, will run a few things on our side and get back with proper review. we do run within kube quite a bit and i'll validate a few things against that env (or if you have availability to do so, feel free). |
| cannot be read (for example on non-Linux hosts, where ``/proc/meminfo`` is | ||
| absent). | ||
| """ | ||
| try: |
There was a problem hiding this comment.
could do
from contextlib import suppress
with suppress(Exception):
...
return None
Problem
_get_vram_allocationssizes available VRAM fromtorch.cuda.mem_get_info().free. On integrated GPUs (such as that on a DGX Spark), that call counts only unallocated pages and ignores the reclaimable page cache. On such a box with most memory held inbuff/cache,freereads only a couple GiB; after the 2 GiB safety buffer the usable fraction collapses to0, and thegpu.vrampreflight check hard-fails every job:…even though tens of GiB are actually reclaimable and usable. Observed on a GB10 with 121 GiB unified memory (
is_integrated == 1), 73 GiB in reclaimable cache,mem_get_info().free ≈ 2 GiB→ check reports~0.0 GiBand fails.Fix
On integrated GPUs, use the kernel's
MemAvailable(from/proc/meminfo, which accounts for reclaimable memory), capped at device total, instead ofmem_get_info's free figure.torch.cuda.get_device_properties(i).is_integrated./proc/meminfocan't be read (e.g. non-Linux hosts).Tests
tests/llm/test_utils.py:get_device_properties);MemAvailable(2 GiB free + 60 GiB available → 58 GiB usable);MemAvailableis unavailable;MemAvailableparsing + absent-file handling.Validated against the modified source with mocked CUDA (discrete
{0: 0.5}unchanged; integrated{0: 0.483}from reclaimable memory).ruff check/ruff format --checkclean.Notes
Summary by CodeRabbit
MemAvailable) rather than relying only on CUDA “free”.