-
Notifications
You must be signed in to change notification settings - Fork 112
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
Pass ASAHI, CUDA, HIP, HSA prefixed env vars to container #526
Conversation
Reviewer's Guide by SourceryThis pull request introduces a change to pass environment variables prefixed with ASAHI, CUDA, HIP, and HSA to the container. It also refactors the code to determine the GPU type and number. Sequence diagram for container setup with GPU environment variablessequenceDiagram
participant C as Container Setup
participant G as get_env_vars()
participant GPU as get_gpu()
participant OS as Operating System
C->>G: Call get_env_vars()
G->>GPU: Call get_gpu()
GPU->>OS: Check /sys/bus/pci/devices for GPU info
OS-->>GPU: Return GPU memory info
alt GPU found
GPU-->>G: Return GPU type and number
else No GPU found
GPU->>OS: Check /etc/os-release
OS-->>GPU: Return OS info
GPU-->>G: Return Asahi or null
end
G->>OS: Get environment variables
OS-->>G: Return matching env vars
G-->>C: Return GPU and env vars
Class diagram for GPU environment handlingclassDiagram
class Common {
+get_gpu(): Tuple[str, int]
+get_env_vars(): Dict[str, str]
+engine_version(engine: str): str
}
class Kube {
-_gen_env_vars(): str
+generate()
}
class Quadlet {
+generate()
}
Kube ..> Common: uses
Quadlet ..> Common: uses
note for Common "Centralized GPU and env var handling"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
I think this is fine, but we have to be mindful of the generators for Quadlet, etc. to ensure the env vars get taken into account there also |
I will push up a change to include those as well. |
@ericcurtin I am curious however, if the better change is to enable explicit |
@sourcery-ai review |
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.
Hey @abn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Relates-to: containers#525 Signed-off-by: Arun Babu Neelicattu <[email protected]>
Relates-to: containers#525 Signed-off-by: Arun Babu Neelicattu <[email protected]>
Relates-to: #525
Summary by Sourcery
Build:
get_env_vars
.