fix: suppress false positive DCGM_FR_NVLINK_DOWN on PCIe GPUs#1481
fix: suppress false positive DCGM_FR_NVLINK_DOWN on PCIe GPUs#1481shaq918 wants to merge 8 commits into
Conversation
PCIe GPUs (A100 PCIe, A40, L40S, etc.) have no NVLink hardware, but DCGM's GpuNvlinkWatch reports all links as "down" with isFatal=true and recommendedAction=RESTART_VM, triggering spurious remediations. Add NVLink hardware detection via a new nvlink_link_count field in GPU metadata. The metadata-collector counts links where NVML GetNvLinkState returns SUCCESS (regardless of ENABLED/DISABLED state) vs NOT_SUPPORTED on PCIe GPUs. The gpu-health-monitor reads this field and suppresses DCGM_FR_NVLINK_DOWN only for GPUs with zero NVLink hardware links. This is conclusive across all SKUs — PCIe GPUs always return NOT_SUPPORTED (count=0), while SXM/NVL GPUs return SUCCESS even for broken links (count>0), so real NVLink failures are never suppressed. Signed-off-by: Mohan Kumar <smohankumar@crusoe.ai> Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
📝 WalkthroughWalkthroughGPU metadata now records total and active NVLink counts. The health monitor evaluates whether NVLink-down incidents are expected for each GPU and suppresses only confirmed false positives while preserving genuine incidents. ChangesNVLink capability flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NVML as CollectNVLinkTopology
participant Metadata as GPU metadata
participant Reader as MetadataReader
participant Watcher as DCGMWatcher
NVML->>Metadata: Store total and active NVLink counts
Watcher->>Reader: Check is_nvlink_down_expected(gpu_id)
Reader->>Metadata: Read and validate GPU metadata
Reader-->>Watcher: Return expected, unexpected, or unknown
Watcher->>Watcher: Skip only expected NVLINK_DOWN incidents
Watcher->>Watcher: Aggregate remaining health status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py`:
- Around line 195-219: Update the suppression logic in _perform_health_check so
removing a DCGM_FR_NVLINK_DOWN incident does not delete the entire GPU’s
aggregated ErrorDetails or other incident codes. Preserve the non-NVLink
incident data while suppressing only the matching failure, and add a regression
test covering a GPU with mixed failure codes.
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py`:
- Around line 189-204: Update the NVLink capability logic around
nvlink_link_count to return True only for authoritative positive integer counts,
and return None when the count is missing, malformed, negative, fractional, or
zero. Remove the nvlinks-list fallback from this decision path so legacy
metadata cannot suppress incidents; preserve warning behavior for invalid
values.
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py`:
- Around line 960-972: Add type hints throughout the new helpers and tests in
the DCGM watcher test section: annotate _make_watcher and _make_metadata_reader
parameters and return types, type the tmp_path fixture with the project’s
appropriate pytest path type, and add -> None to every test function from the
affected range.
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py`:
- Around line 325-463: Add complete type annotations to each new test function
in the has_nvlink test group, including -> None return annotations and Path
annotations for any fixture parameters such as tmp_path. Import Path if needed,
while preserving the existing test behavior and setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a0174a98-4e06-42a5-bec4-a2f7a9c85a9c
📒 Files selected for processing (8)
data-models/pkg/model/gpu_metadata.gohealth-monitors/gpu-health-monitor/gpu_health_monitor/cli.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.pymetadata-collector/pkg/nvml/topology.gotests/helpers/metadata.go
The legacy nvlinks list only records NVSwitch-connected links and would miss GPU-to-GPU NVLink (e.g., H100 NVL). Return None when nvlink_link_count is missing so suppression stays disabled until the authoritative field is available from an upgraded metadata-collector. Addresses CodeRabbit review feedback. Signed-off-by: Mohan Kumar <smohankumar@crusoe.ai> Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
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)
health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py (1)
167-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring: fallback to
nvlinksno longer exists.The docstring still says it falls "back to the nvlinks list for older metadata-collector versions," but the implementation below (lines 192-197) explicitly returns
Noneinstead of falling back. Update the docstring to reflect the fail-closed behavior.📝 Proposed fix
def has_nvlink(self, gpu_id: int) -> Optional[bool]: """Check whether the GPU has NVLink hardware. - Uses the nvlink_link_count field (number of NVLink links the hardware - supports, regardless of current link state) when available, falling - back to the nvlinks list for older metadata-collector versions. + Uses the nvlink_link_count field (number of NVLink links the hardware + supports, regardless of current link state). If the field is missing + or invalid (e.g., old metadata-collector versions), returns None so + callers treat NVLink capability as unknown rather than assuming a + legacy nvlinks list is authoritative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py` around lines 167 - 169, Update the docstring for the NVLink metadata reader to remove the claim that it falls back to the nvlinks list. Document that when nvlink_link_count is unavailable, the reader returns None instead.
🧹 Nitpick comments (1)
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py (1)
447-467: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for negative/fractional
nvlink_link_countvalues.The invalid-value test only exercises a non-numeric string (
"invalid"). Once the fail-closed numeric-range fix is applied inreader.py, add cases for negative (-1) and fractional (0.5) counts to lock in the fixed behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py` around lines 447 - 467, Add coverage in test_has_nvlink_invalid_nvlink_link_count_returns_none for numeric invalid values -1 and 0.5, asserting MetadataReader.has_nvlink(0) returns None for each case. Reuse the existing metadata/temp-file setup while varying nvlink_link_count, preserving the current non-numeric invalid-value assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py`:
- Around line 191-207: Validate nvlink_link_count in the GPU metadata handling
before converting it to an integer: reject negative values and fractional
numeric values as unknown, returning None and logging the existing warning.
Update the logic in the nvlink_link_count branch while preserving positive
integers as True and zero as False.
---
Outside diff comments:
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py`:
- Around line 167-169: Update the docstring for the NVLink metadata reader to
remove the claim that it falls back to the nvlinks list. Document that when
nvlink_link_count is unavailable, the reader returns None instead.
---
Nitpick comments:
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py`:
- Around line 447-467: Add coverage in
test_has_nvlink_invalid_nvlink_link_count_returns_none for numeric invalid
values -1 and 0.5, asserting MetadataReader.has_nvlink(0) returns None for each
case. Reuse the existing metadata/temp-file setup while varying
nvlink_link_count, preserving the current non-numeric invalid-value assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d58155c-b6dc-40fd-b09b-de94c8b9ced5
📒 Files selected for processing (2)
health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py
|
hi @shaq918 , thank you for your PR! Can you please confirm which version of DCGM you are using? |
|
Hi @lalitadithya, thanks for the review! We observed this on DCGM 3.3.7 (via the gpu-operator DCGM service) running on A100 PCIe 80GB nodes. The The behavior should be the same on DCGM 4.x since the NVLink health watch has not changed its reporting semantics between versions — PCIe GPUs genuinely have no NVLink hardware, so DCGM correctly reports the links as down. The issue is that NVSentinel treats this as a failure requiring remediation. The fix is version-independent: it uses NVML |
|
/ok to test 0155d3b |
Ahh.. I see, okay. |
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1481.docs.buildwithfern.com/nvsentinel |
|
@XRFXLP could you take a look at this one as well when you get a chance? |
|
@shaq918 Can you please address CI failures and CR comments? |
- Suppress DCGM_FR_NVLINK_DOWN per incident inside _perform_health_check instead of deleting the aggregated (watch, GPU) entry, which dropped genuine co-occurring incidents on the same GPU. - Fail closed on malformed nvlink_link_count: only a non-negative integer confirms capability; bool/float/string/negative values read as unknown and never suppress. - Make NVLinkLinkCount *int with omitempty: the collector appends the GPU even when NVLink topology collection fails, so a missing field must read as unknown downstream, not as confirmed-no-NVLink (which 0 means). - Rewrite suppression tests to drive _perform_health_check, add a mixed-code regression test, and add type hints to new tests. - Format with Black and gofmt to fix CI lint failures. Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
|
/ok to test 26407c3 |
Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
|
/ok to test e4a063b |
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
|
hey @shaq918,
We've few L40/L40S clusters that have PCIe interconnect, but this issue is not present there. And nvlinks is empty: Driver version is 580.95.05, DCGM being: But we don't have any node condition for this: So on this stack: L40 (PCIe), driver 580.95.05, DCGM 4.3.1, GpuNvlinkWatch is enabled (you can see it initialize the watch and update the cache for GpuNvlinkWatch|GPU|0), yet DCGM's passive health check reports it healthy: the node condition sits at That's the crux. The only place a Could you share a concrete repro so we can see where the incident actually originates?
If it came from |
Live probe on an affected A100 80GB PCIe (unbridged) showed NVML
GetNvLinkState returns SUCCESS with state NOT_ACTIVE for links 0-11:
the hardware count is 12, not 0, so hardware-count-based suppression
could never fire on the SKU that actually produces the false positive.
Only GPUs with no NVLink silicon (L40/A40) report 0, and DCGM never
fires NVLINK_DOWN on those anyway.
Collect nvlink_active_link_count (links in FEATURE_ENABLED state) next
to the hardware count, and suppress DCGM_FR_NVLINK_DOWN only when the
GPU verifiably has zero active links by design:
- active == 0 and hardware == 0 -> no NVLink silicon
- active == 0 and device name has "PCIe" -> unbridged bridge-capable
card (bridge links train at driver load, no fabric manager, so a
zero reading is trustworthy)
- active == 0 otherwise -> unknown: on SXM/HGX the
links train via fabric manager after boot, so never suppress
- active > 0 -> genuine fault, never
suppress
Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
|
@XRFXLP Great questions — they made us re-verify everything on a live node, and that both produced the repro you asked for and exposed a flaw in the fix itself (now corrected in 66f9427). Full detail below. Concrete repro (captured live yesterday)Stack: NVIDIA A100 80GB PCIe (single GPU, no NVLink bridge installed), driver 570.133.20, standalone hostengine It is the passive health watch, not which flows through The monitor's logs also show it flapping (fires → clears → re-fires within ~15s). Why your L40/L40S clusters don't see itThis was the key insight. L40 has no NVLink silicon at all, while A100 PCIe is NVLink-bridge-capable — the 12 bridge links physically exist, they're just permanently inactive when no bridge is installed. Raw NVML probe on the affected A100 PCIe ( On L40 every link returns NOT_SUPPORTED, so DCGM's NVLink watch has nothing to monitor and stays PASS — exactly what you observe. On A100 PCIe, DCGM sees 12 real links that are all down and reports that steady state as a FATAL fault with RESTART_VM. What this changed in the PRThe same probe falsified the PR's original premise ("GetNvLinkState returns NOT_SUPPORTED on PCIe → count 0"): the hardware link count on an unbridged A100 PCIe is 12, so hardware-count-based suppression could never have fired on the SKU that actually produces the false positive. Fixed in 66f9427:
Tests cover the exact repro shape (hardware=12, active=0, "PCIe" name), the SXM zero-active fail-closed case, and mixed-code aggregation on the same GPU. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py (1)
1052-1066: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a bridged PCIe card with partial active links.
Coverage exists for the "zero active links" suppression case (this test) and for SXM's fail-closed zero-active-links case (
test_no_suppress_sxm_gpu_zero_active_links), but there's no test for a PCIe-named GPU withnvlink_active_link_count > 0that still receives aDCGM_FR_NVLINK_DOWNincident on an inactive link. This is a realistic scenario — bridged PCIe NVLink cards can legitimately have a subset of links active while others are down or standby — and the suppression condition (no active links AND (no hardware OR PCIe name)) should be verified to NOT suppress in this case, since it would otherwise mask a genuine partial failure.✅ Suggested additional test
def test_no_suppress_pcie_gpu_with_partial_active_links(self, tmp_path: Path) -> None: """PCIe card with some active links (e.g. bridged, partially trained): NVLINK_DOWN on a different link is a genuine failure, NOT suppressed.""" gpu = dict(self.A100_PCIE_UNBRIDGED, nvlink_active_link_count=6) reader = self._make_metadata_reader(tmp_path, [gpu]) watcher = self._make_watcher(metadata_reader=reader) health_status = self._run_health_check(watcher, [self._nvlink_down_incident(0, 8)]) details = health_status["DCGM_HEALTH_WATCH_NVLINK"] assert details.status == dcgm.types.HealthStatus.FAIL assert details.entity_failures[0].code == "DCGM_FR_NVLINK_DOWN"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py` around lines 1052 - 1066, Add a test alongside test_suppress_unbridged_pcie_card covering a PCIe GPU with nvlink_active_link_count greater than zero and an NVLINK_DOWN incident on an inactive link. Build the metadata from A100_PCIE_UNBRIDGED with a partial active-link count, run the health check, and assert the NVLink watch reports FAIL with a DCGM_FR_NVLINK_DOWN entity failure rather than suppressing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py`:
- Around line 228-234: Format the new Python code with Black: wrap the
link-count helper calls in
health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py lines
228-234, and wrap the test signatures at lines 405, 412, and 420 in
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py.
- Around line 208-209: Clarify the fail-closed contract near the metadata count
logic to apply specifically to missing or malformed active-link counts, while
documenting the existing PCIe exception where nvlink_link_count is unknown and
device_name identifies PCIe. Ensure the wording does not imply all missing
counts return None when the PCIe naming rule authoritatively returns True.
---
Nitpick comments:
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py`:
- Around line 1052-1066: Add a test alongside test_suppress_unbridged_pcie_card
covering a PCIe GPU with nvlink_active_link_count greater than zero and an
NVLINK_DOWN incident on an inactive link. Build the metadata from
A100_PCIE_UNBRIDGED with a partial active-link count, run the health check, and
assert the NVLink watch reports FAIL with a DCGM_FR_NVLINK_DOWN entity failure
rather than suppressing it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e56638c0-51a7-452b-ac14-0f7711e2ce5d
📒 Files selected for processing (7)
data-models/pkg/model/gpu_metadata.gohealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.pymetadata-collector/pkg/nvml/topology.gotests/helpers/metadata.go
🚧 Files skipped from review as they are similar to previous changes (3)
- metadata-collector/pkg/nvml/topology.go
- tests/helpers/metadata.go
- health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py
The active count always fails closed; the hardware count is corroborating only — a PCIe device name substitutes for it. Signed-off-by: Shashank Mohankumar <smohankumar@crusoe.ai>
|
/ok to test 5a95603 |
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
hey @shaq918, Thanks for the follow-up fix and the detailed explanation.▎ I'd want to tighten this case: a bridge-capable non-SXM board whose bridge is dead or absent at collection time falls under this exact umbrella (same device name, same active == 0), so a genuine bridge failure present at boot would be silently suppressed. And since collection is one-shot at startup, it stays suppressed until the collector restarts. Since an unbridged card and a card with a dead bridge are indistinguishable from the metadata alone, could we make this branch express intentionality via configuration rather than infer it from the device name? Concretely:
That keeps the default fail-safe (no real NVLink fault is ever masked without someone explicitly declaring the pool bridgeless), while still giving unbridged-PCIe fleets the suppression they want. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py`:
- Around line 970-995: Centralize the duplicated NVLink fixtures and
metadata-reader helper in a shared test fixtures module. In
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py
lines 326-357, move A100_PCIE_UNBRIDGED, H100_SXM_TRAINED, L40_NO_NVLINK, and
_reader_for to that module and import them; in
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py
lines 970-995, remove the local class-attribute copies and import the shared
fixtures (or use ClassVar if any remain local).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4593ee44-dfbb-4ee9-bcfb-7b198fa57049
📒 Files selected for processing (7)
data-models/pkg/model/gpu_metadata.gohealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.pymetadata-collector/pkg/nvml/topology.gotests/helpers/metadata.go
🚧 Files skipped from review as they are similar to previous changes (5)
- metadata-collector/pkg/nvml/topology.go
- data-models/pkg/model/gpu_metadata.go
- tests/helpers/metadata.go
- health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py
- health-monitors/gpu-health-monitor/gpu_health_monitor/metadata/reader.py
| A100_PCIE_UNBRIDGED = { | ||
| "gpu_id": 0, | ||
| "uuid": "GPU-0", | ||
| "device_name": "NVIDIA A100 80GB PCIe", | ||
| "nvlinks": [], | ||
| "nvlink_link_count": 12, | ||
| "nvlink_active_link_count": 0, | ||
| } | ||
|
|
||
| H100_SXM_TRAINED = { | ||
| "gpu_id": 0, | ||
| "uuid": "GPU-0", | ||
| "device_name": "NVIDIA H100 80GB HBM3", | ||
| "nvlinks": [], | ||
| "nvlink_link_count": 18, | ||
| "nvlink_active_link_count": 18, | ||
| } | ||
|
|
||
| L40_NO_NVLINK = { | ||
| "gpu_id": 0, | ||
| "uuid": "GPU-0", | ||
| "device_name": "NVIDIA L40", | ||
| "nvlinks": [], | ||
| "nvlink_link_count": 0, | ||
| "nvlink_active_link_count": 0, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated NVLink test fixtures/helper across two test files. Both files independently define identical A100_PCIE_UNBRIDGED/H100_SXM_TRAINED/L40_NO_NVLINK dicts and a "write JSON metadata to tmp_path, build a MetadataReader" helper. One root cause — no shared fixtures module — with two symptoms (copy/paste drift risk, and RUF012 in the class-attribute copy).
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py#L970-L995: replace these class attributes with an import from a shared fixtures module (or addClassVarif kept local), removing the copy of the fixtures.health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py#L326-L357: moveA100_PCIE_UNBRIDGED,H100_SXM_TRAINED,L40_NO_NVLINK, and_reader_forinto that shared module and import them here instead of redefining.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 970-977: Mutable default value for class attribute
(RUF012)
[warning] 979-986: Mutable default value for class attribute
(RUF012)
[warning] 988-995: Mutable default value for class attribute
(RUF012)
📍 Affects 2 files
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py#L970-L995(this comment)health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py#L326-L357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py`
around lines 970 - 995, Centralize the duplicated NVLink fixtures and
metadata-reader helper in a shared test fixtures module. In
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_metadata/test_reader.py
lines 326-357, move A100_PCIE_UNBRIDGED, H100_SXM_TRAINED, L40_NO_NVLINK, and
_reader_for to that module and import them; in
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py
lines 970-995, remove the local class-attribute copies and import the shared
fixtures (or use ClassVar if any remain local).
3 participants
Summary
isFatal=trueandrecommendedAction=RESTART_VM, triggering spurious remediationsnvlink_link_countin GPU metadata and suppressDCGM_FR_NVLINK_DOWNonly for GPUs with zero NVLink hardware linksGetNvLinkStatereturn codes (NOT_SUPPORTEDon PCIe vsSUCCESSon SXM/NVL even for broken links)Changes
Data Model
nvlink_link_countfield toGPUInfo— counts NVLink links the GPU hardware supports regardless of link stateMetadata Collector
GetNvLinkStatereturnsSUCCESS(hardware present) inCollectNVLinkTopologyNOT_SUPPORTED), SXM/NVL → >0 (returnSUCCESSeven if links are down)GPU Health Monitor
MetadataReader.has_nvlink()usingnvlink_link_countwith fallback tonvlinkslist_suppress_nvlink_down_on_pcie_gpus()to suppress false positive NVLink-down on PCIe GPUsMetadataReadercreation unconditional (was gated behindthermal_margin_enabled)Tests
has_nvlink()(positive count, zero count, fallback, missing fields, unknown GPU, missing file, invalid value)Test plan
has_nvlink()logic verified locallynvlink_link_count: 0on PCIe node,DCGM_FR_NVLINK_DOWNevents suppressednvlink_link_count: >0on SXM node, events NOT suppressedSummary by CodeRabbit
nvlink_link_count(total) andnvlink_active_link_count(active), enabling more accurate GPU characterization.