-
Notifications
You must be signed in to change notification settings - Fork 0
MILAB-6382: expose host NVIDIA driver via runtime envs #91
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@platforma-open/milaboratories.runenv-python-3.12.10-rapids': minor | ||
| --- | ||
|
|
||
| NVIDIA env vars |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,11 @@ | |
| "registry": "platforma-open", | ||
| "python-version": "3.12.10", | ||
| "envVars": [ | ||
| "RPY2_CFFI_MODE=ABI" | ||
| "RPY2_CFFI_MODE=ABI", | ||
| "NVIDIA_VISIBLE_DEVICES=all", | ||
| "NVIDIA_DRIVER_CAPABILITIES=compute,utility", | ||
| "LD_LIBRARY_PATH=/usr/local/nvidia/lib64:/usr/local/nvidia/lib", | ||
| "PATH=/usr/local/nvidia/bin:${PATH}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Every other Prompt To Fix With AIThis is a comment left during a code review.
Path: python-3.12.10-rapids/package.json
Line: 29
Comment:
**`${PATH}` expansion not guaranteed in JSON `envVars`**
Every other `envVars` entry in this repo is a plain `KEY=VALUE` literal with no variable references. If the Platforma runtime sets these strings directly (e.g. via `setenv`) rather than passing them through a shell, `PATH` will be set to the literal string `/usr/local/nvidia/bin:${PATH}` — with the unexpanded text `${PATH}` as the suffix. That would break resolution of every binary (including `python` itself) that lives outside `/usr/local/nvidia/bin`, making the entire run environment non-functional. Please confirm that the runtime performs `${...}` interpolation before applying `envVars`, or replace this entry with a fully resolved static path.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: python-3.12.10-rapids/package.json
Line: 28-29
Comment:
**NVIDIA runtime vars applied to all platforms including macOS and Windows**
`NVIDIA_VISIBLE_DEVICES`, `NVIDIA_DRIVER_CAPABILITIES`, and the `/usr/local/nvidia/...` paths are NVIDIA Container Runtime constructs that are only meaningful in Linux containers with GPU passthrough. The `roots` in this package include `macosx-x64`, `macosx-aarch64`, and `windows-x64`. On those platforms the paths won't exist and `LD_LIBRARY_PATH` will point to directories that are absent, potentially interfering with any loader that respects `LD_LIBRARY_PATH` on macOS (`DYLD_LIBRARY_PATH` is the macOS equivalent). If RAPIDS GPU features are explicitly unsupported on non-Linux platforms, consider conditionally applying these vars only for Linux targets (if the runtime supports platform-specific env vars), or document that the NVIDIA vars are no-ops on non-Linux platforms.
How can I resolve this? If you propose a fix, please make it concise. |
||
| ], | ||
| "roots": { | ||
| "linux-x64": "./pydist/linux-x64", | ||
|
|
||
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.
LD_LIBRARY_PATHsilently overwrites any pre-existing valueUnlike
PATH, which at least attempts to incorporate the existing value via${PATH},LD_LIBRARY_PATHis set to a fixed string. If the host container (or any previously applied runenv layer) already exportsLD_LIBRARY_PATHentries (e.g. CUDA toolkit, cuDNN), those entries will be silently dropped, potentially causing runtime linker failures for libraries that depend on them. Using the same${LD_LIBRARY_PATH}pattern keeps existing paths intact — assuming the runtime supports interpolation.Prompt To Fix With AI