-
Notifications
You must be signed in to change notification settings - Fork 853
fix: CI is broken with a deprecated dependency on pynvml #2926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Harrison Saturley-Hall <[email protected]>
Signed-off-by: Harrison Saturley-Hall <[email protected]>
WalkthroughDependencies updated by replacing pynvml with nvidia-ml-py and removing a license header in requirements. Test configuration updated to suppress a pynvml deprecation FutureWarning in PyTest via pyproject.toml. No runtime code or public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed Checks (1 warning)
✅ Passed Checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rmccorm4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right fix? I see the pynvml related failure still happening on this PR's check: https://github.com/ai-dynamo/dynamo/actions/runs/17555361883/job/49858015783?pr=2926#step:8:680
Signed-off-by: alec-flowers <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
container/deps/requirements.txt (1)
19-19: Document the rationale and lock intent next to the pin.Add a short inline comment so future updaters know this replaces the deprecated wheel and that the import name is still
pynvml.-nvidia-ml-py==13.580.65 +nvidia-ml-py==13.580.65 # replaces deprecated `pynvml`; module import remains `pynvml`pyproject.toml (1)
156-156: Narrow the warning filter to just thepynvmlmodule to avoid masking unrelated FutureWarnings.Current pattern-wide ignore can hide similarly worded messages. Scope the filter by module to keep the global
errorpolicy effective elsewhere. Keep the “temporary” note.- "ignore:The pynvml package is deprecated.*:FutureWarning", # Ignore pynvml deprecation warning, temporary until upstream library updates to nvidia-ml-py + "ignore:The pynvml package is deprecated.*:FutureWarning:pynvml", # Temporary: ignore only when originating from the pynvml module
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
container/deps/requirements.txt(1 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test - vllm
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (3)
container/deps/requirements.txt (2)
19-19: Replace pynvml with nvidia-ml-py — good, API remains importable aspynvml.This aligns with NVIDIA’s deprecation guidance and should unblock CI without runtime code changes.
19-19: Compatibility Verified for nvidia-ml-py Pin
nvidia-ml-py 13.580.65 is published on PyPI as a universalpy3-none-any.whland source tarball, covering Python 3.10–3.12, and no imports or dependencies onpynvmlremain in the codebase or lock files.pyproject.toml (1)
124-157: Skip adding annvmloptional extra—nopynvmlimports detected
Ripgrep search across all Python files (excluding tests, vendor, and third_party) found no unconditionalimport pynvmlorfrom pynvmlusages; no runtime NVML dependency exists.
Signed-off-by: Harrison Saturley-Hall <[email protected]> Signed-off-by: alec-flowers <[email protected]> Co-authored-by: alec-flowers <[email protected]> Signed-off-by: Harrison King Saturley-Hall <[email protected]>
Signed-off-by: Harrison Saturley-Hall <[email protected]> Signed-off-by: alec-flowers <[email protected]> Signed-off-by: Harrison King Saturley-Hall <[email protected]> Co-authored-by: alec-flowers <[email protected]>
Signed-off-by: Harrison Saturley-Hall <[email protected]> Signed-off-by: alec-flowers <[email protected]> Co-authored-by: alec-flowers <[email protected]> Signed-off-by: Indrajit Bhosale <[email protected]>
Signed-off-by: Harrison Saturley-Hall <[email protected]> Signed-off-by: alec-flowers <[email protected]> Co-authored-by: alec-flowers <[email protected]>
Signed-off-by: Harrison Saturley-Hall <[email protected]> Signed-off-by: alec-flowers <[email protected]> Co-authored-by: alec-flowers <[email protected]> Signed-off-by: hongkuanz <[email protected]>
Overview:
pynvmlis deprecated and is breaking CI. It is recommended to be replaced withnvidia-ml-pyRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Chores
Tests
Notes