build: add pynvml to requirements.txt #1480
Conversation
hhzhang16
requested review from
rmccorm4,
tanmayv25,
richardhuo-nv,
ptarasiewiczNV,
ishandhanani,
alec-flowers,
nnshah1 and
a team
as code owners
WalkthroughThe dependency file was updated to include the Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
container/deps/requirements.txt (1)
33-33: Ensure compatibility by pinningpynvmlto a known versionCurrently
pynvmlis added without a version specifier, which can lead to unpredictable installs and GPU driver mismatches. Pin it to the version verified in your development container, for example:-pynvml +pynvml==11.7.0
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/deps/requirements.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: copyright-checks
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-commit
🔇 Additional comments (1)
container/deps/requirements.txt (1)
1-43: Verify that CI and dev container use this requirements fileConfirm that your Dockerfile, CI pipelines, or any bootstrap scripts are updated to install dependencies from
container/deps/requirements.txt. Without referencing this file,pynvmlwon’t be installed and GPU discovery/SLA profiling will continue to fail.
rmccorm4
changed the title
There was a problem hiding this comment.
pynvml was added to dynamo deploy 2 months ago and is required for GPU use and discovery for dynamo deploy as well as SLA profiling. If not included, GPU functionality within the devcontainer is limited.
Where was it installed previously for the past 2 months of dev/testing this feature?
There was a problem hiding this comment.
@biswapanda @tedzhouhk I think you two were responsible for some of the Pynvml-related code contributions!
There was a problem hiding this comment.
@nvda-mesharma @saturley-hall @nv-anants can you help to define some guidelines on when things should be installed in container-only (ex: container/deps/requirements.txt) vs. installed in pyproject.toml?
I think we should avoid diverging core dynamo feature support from pip install ai-dynamo and docker run ... <dynamo_image> paths as much as possible.
container/deps/requirements.txt
Outdated
| @@ -30,6 +30,7 @@ pip==25.0.1 | |||
| pre-commit | |||
| protobuf==5.27.3 | |||
| pydantic==2.7.1 | |||
| pynvml | |||
There was a problem hiding this comment.
@biswapanda what version do you want to freeze it at?
container/deps/requirements.txt
Outdated
| @@ -30,6 +30,7 @@ pip==25.0.1 | |||
| pre-commit | |||
| protobuf==5.27.3 | |||
| pydantic==2.7.1 | |||
| pynvml | |||
There was a problem hiding this comment.
-
in general, we'd freeze version to avoid backward incompatibility / version drift
-
we don't need
pynvmlto usepynvmlmodule, it sounds confusing -
nvidia-ml-pypackage installspynvmlmodule
pynvmlpackage installspynvml_utils(we dont use this)
Overview:
pynvml was added to dynamo deploy 2 months ago and is required for GPU use and discovery for dynamo deploy as well as SLA profiling. If not included, GPU functionality within the devcontainer is limited.
Details:
Places pynvml is referenced in the code:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
nvidia-ml-pypackage.