-
Notifications
You must be signed in to change notification settings - Fork 94
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
Make the default of ngl be -1 #707
base: main
Are you sure you want to change the base?
Conversation
This means automatically assign a value, which may be 999 or 0 depending on hardware. Signed-off-by: Eric Curtin <[email protected]>
Reviewer's Guide by SourceryThe pull request changes the default value of the State diagram for GPU layer configurationstateDiagram-v2
[*] --> CheckNGL
CheckNGL --> AutoAssign: ngl = -1
CheckNGL --> ManualConfig: ngl specified
AutoAssign --> HardwareDetection
HardwareDetection --> CUDA: CUDA/HIP detected
HardwareDetection --> Other: Other GPU
CUDA --> [*]: Set 999 layers
Other --> [*]: Set 0 layers
ManualConfig --> [*]: Use specified value
Flow diagram for GPU layer assignment logicgraph TD
A[Start] --> B{ngl = -1?}
B -->|Yes| C[Auto-assign GPU layers]
C --> D{Hardware Type}
D -->|CUDA/HIP| E[Set to 999 layers]
D -->|Other GPU| F[Set to 0 layers]
B -->|No| G[Use specified ngl value]
E --> H[End]
F --> H
G --> H
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:
Overall Comments:
- Please document the valid values for the gpu parameter and what each value means (especially -1, 0, and positive values)
- The change from
args.gpu
toargs.gpu > 0
in gpu_args() could be a breaking change. Please explain the rationale and document the behavior change
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.
Needed to access the gpu on some systems, but has an impact on security, use with caution. | ||
|
||
#### **--ngl** | ||
number of gpu layers (default: 999) | ||
number of gpu layers (default: -1) |
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.
suggestion: Clarify the meaning of -1 for ngl.
The documentation should explicitly state what a value of -1 means for the number of GPU layers. Does it mean "use all available layers" or something else? Also, "gpu" should be "GPU".
Needed to access the gpu on some systems, but has an impact on security, use with caution. | |
#### **--ngl** | |
number of gpu layers (default: 999) | |
number of gpu layers (default: -1) | |
Needed to access the GPU on some systems, but has an impact on security, use with caution. | |
#### **--ngl** | |
number of GPU layers (default: -1). A value of -1 means use all available GPU layers |
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.
I agree with sorcery, you need to explain the value of -1?
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.
Thanks @rhatdan! I'm glad you agree. @, it's important to make this clear in the documentation so users understand how to use this option. Please update the documentation to explain the meaning of -1, as suggested in my previous comment. Let me know if you have any questions about how to best phrase this.
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.
+1 magic numbers are bad.
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.
Thank you for your feedback, we will generate more comments like this in the future.
@@ -51,7 +51,7 @@ | |||
|
|||
# Default number of layers offloaded to the gpu | |||
# | |||
#ngl = 999 | |||
#ngl = -1 |
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.
Explain -1 above
@@ -92,7 +92,7 @@ RAMALAMA_IMAGE environment variable overrides this field. | |||
Pass `--group-add keep-groups` to podman, when using podman. | |||
In some cases this is needed to access the gpu from a rootless container | |||
|
|||
**ngl**=999 | |||
**ngl**=-1 |
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.
Please explain the meaning of -1.
This means automatically assign a value, which may be 999 or 0 depending on hardware.
Summary by Sourcery
Documentation:
ngl
).