-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add ramalama gpu_detector #670
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new feature to detect and display available GPUs on the system. It adds a new command-line argument Sequence diagram for GPU detection flowsequenceDiagram
participant User
participant CLI
participant GPUDetector
participant System
User->>CLI: ramalama --show-gpus-available
CLI->>GPUDetector: create GPUDetector()
CLI->>GPUDetector: detect_best_gpu(gpu_list)
GPUDetector->>System: Check NVIDIA GPUs (nvidia-smi)
GPUDetector->>System: Check AMD GPUs (sysfs)
GPUDetector->>System: Check Intel GPUs (sysfs/lspci)
GPUDetector-->>CLI: Return detection result
CLI-->>User: Display available GPUs
Class diagram for the new GPUDetectorclassDiagram
class GPUDetector {
-best_gpu: str
-best_vram: int
-best_env: str
+__init__()
-_update_best_gpu(memory_path: str, gpu_index: int, env_var: str)
+get_nvidia_gpu()
+get_amd_gpu()
+get_intel_gpu()
+detect_best_gpu(gpu_template: list) bool
}
note for GPUDetector "Detects and compares GPU capabilities
Supports NVIDIA, AMD, and Intel GPUs"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @dougsland - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the bytes to MiB conversion logic into a utility function to avoid duplication and ensure consistent handling across different GPU detection methods
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
b704695
to
a8e9e06
Compare
cc Brian [email protected] |
in a different patch will take care of macOS env, need to test it but it's very later now 2AM. |
4219667
to
d620147
Compare
CI/CD error seems not related to the PR. |
I don't have permission to re-run CI/CD. Someone can re-run please? @ericcurtin |
c449866
to
7617f70
Compare
Thinking here I believe maybe instead of ERROR we should print INFO to avoid confusion. When found one GPU and try to get the others but don't find. @rhatdan @ericcurtin. At the same time, it's getting more and more code, I would merge this one and guarantee we have something tested fine and a next patch I change to INFO if all agree with the idea. |
GPUDetector can be used in any part of the code. However, useful for users to run in the cli. ramalama info Signed-off-by: Douglas Schilling Landgraf <[email protected]> Signed-off-by: Brian <[email protected]>
7617f70
to
8d27050
Compare
Already changed to INFO, should be all set. |
Intel that I have:
macOS:
|
This looks great. Thank you for the contribution and for addressing all the feedback. 😄 |
My pleasure @maxamillion , looking forward to contribute more. Please share issues that you guys looking for help. |
Great work @dougsland LGTM |
GPUDetector can be used in any part of the code.
However, useful for users to run in the cli.
ramalama --show-gpus-available
Summary by Sourcery
New Features:
--show-gpus-available
CLI flag.