-
Notifications
You must be signed in to change notification settings - Fork 234
Do not override a small subset of env vars #1475
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
Conversation
Reviewer's GuideThis PR refactors and simplifies how accelerator environment variables and base image resolution are handled by renaming, narrowing, and restructuring related functions, extracting common logic into a new utility, and streamlining accel_image’s flow. 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 @ericcurtin - I've reviewed your changes - here's some feedback:
- In accel_image, you’re iterating over
env_vars
instead ofgpu_type_env_vars
, which will cause a reference error or skip GPU detection—please fix the variable reference. - The docstring for accel_image has an extra "and"—change "Selects and the appropriate image" to "Selects the appropriate image" for clarity.
- The separation between get_gpu_type_env_vars and get_accel_env_vars introduces some duplication; consider merging or renaming them to more clearly convey their distinct roles.
Here's what I looked at during the review
- 🟡 General issues: 2 issues 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.
RamaLama does not try to detect GPU if the user has already set certain env vars. Make this list smaller. Signed-off-by: Eric Curtin <[email protected]>
LGTM |
RamaLama does not try to detect GPU if the user has already set certain env vars. Make this list smaller.
Summary by Sourcery
Restrict automatic environment variable overrides to core GPU settings and refactor image resolution by extracting a dedicated resolver and simplifying GPU-based selection
Enhancements: