-
Notifications
You must be signed in to change notification settings - Fork 62
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
code crashes for rocm added proper type cast for env var #602
Conversation
Signed-off-by: Brian <[email protected]>
Reviewer's Guide by SourceryThe pull request addresses a potential code crash by ensuring that the environment variable 'HIP_VISIBLE_DEVICES' is set to a string value. This is achieved by explicitly casting the 'gpu_num' variable to a string before assigning it to the environment variable. Sequence diagram for ROCm GPU device selectionsequenceDiagram
participant Code as Application Code
participant GPU as get_gpu() Function
participant Env as Environment Variables
Code->>GPU: Call get_gpu()
GPU->>GPU: Process GPU bytes
Note over GPU: Convert gpu_num to string
GPU->>Env: Set HIP_VISIBLE_DEVICES=str(gpu_num)
GPU-->>Code: Return
Flow diagram for GPU device selection with type castingflowchart TD
A[Start] --> B{GPU bytes available?}
B -->|Yes| C[Convert gpu_num to string]
C --> D[Set HIP_VISIBLE_DEVICES environment variable]
D --> E[Return]
B -->|No| E
E[Return]
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 @bmahabirbu - I've reviewed your changes - here's some feedback:
Overall Comments:
- While the fix is correct, please consider using a more descriptive commit message that explains the specific issue (type mismatch when setting HIP_VISIBLE_DEVICES environment variable) and why the fix was needed.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
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.
Sometimes when writing python I really miss having a compiler, it would catch issues like this!
@maxamillion and @bmahabirbu ye both have maintainer permissions, feel free to merge green PRs that don't have issues etc. I'm gonna sleep. |
Agreed! My fault initally, I should have tested this on my amd machine. Was too caught up with cuda |
Awesome! |
If we can find static analysis tools that are capable of detecting things like this it would be great if we added, many humans would miss this in review. Or some extra tests that execute these code bits. Or a unit test would be perfect. I actually noticed quay.io rocm pull stats dipped a little, makes sense now. |
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.
LGTM!
Hey @ericcurtin while checking into rocm detection I realized this can break the code so here is a quick fix!
Summary by Sourcery
Bug Fixes: