-
Notifications
You must be signed in to change notification settings - Fork 107
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
Check for apple,arm-platform in /proc #730
Conversation
Does this work for? |
Reviewer's Guide by SourceryThis pull request modifies the get_gpu function to check for the "asahi" string in /proc/cmdline instead of /etc/os-release to improve portability across Fedora Asahi Remix and Ubuntu Asahi Remix. Sequence diagram for updated GPU detection processsequenceDiagram
participant App
participant get_gpu
participant FileSystem
App->>get_gpu: Check GPU
get_gpu->>FileSystem: Check /proc/cmdline exists
alt File exists
FileSystem-->>get_gpu: Yes
get_gpu->>FileSystem: Read /proc/cmdline
FileSystem-->>get_gpu: File contents
alt Contains 'asahi'
get_gpu->>get_gpu: Set ASAHI_VISIBLE_DEVICES=1
end
end
get_gpu-->>App: Return
File-Level Changes
Possibly linked issues
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 @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Using /proc/cmdline to detect Asahi Linux is less reliable than checking /etc/os-release. The kernel command line is an implementation detail that could change, while /etc/os-release is the standard location for OS distribution information. Consider keeping the original check or implementing both checks if 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.
A better way might be to set an environment variable within the image to identify the image type. One issue with doing it based on command line, is that someone could launch service on a different container based on the asahi container. Lets change the containerfiles to set the GPU type as an environment variable. ENV ASAHI_VISIBLE_DEVICES 1 During the build, then during the run, there is no issue. |
e01cb9f
to
d908dd2
Compare
d908dd2
to
afae544
Compare
Added |
In /proc/device-tree/compatible specifically, thanks Asahi Lina! It's more portable accross Fedora Asahi Remix and Ubuntu Asahi Remix. Also added env var to container image. Co-authored-by: Asahi Lina <[email protected]> Co-authored-by: Daniel J Walsh <[email protected]> Signed-off-by: Eric Curtin <[email protected]>
afae544
to
c9c4a60
Compare
I don't fully understand what is going on here, but this does look like an improvement over what we had before. I trust the asahi people will test. |
#526 (comment) |
Yeah sometimes the diagrams are quite cool, I don't always take them for granted though. They are AI generated at the end of the day. |
if we ignore the env var for a second. It's because we need a way of detecting we are running on Asahi to download the Asahi container image. I initially put in a way that was only compatible with Fedora Asahi Remix. The new way is more portable and can detect we need to pull the Asahi container image on Ubuntu Asahi Remix. |
In /proc/device-tree/compatible specifically, thanks Asahi Lina!
It's more portable accross Fedora Asahi Remix and Ubuntu Asahi
Remix.
Also added env var to container image.
Co-authored-by: Asahi Lina [email protected]
Co-authored-by: Daniel J Walsh [email protected]