add --ngl to specify the number of gpu layers, and --keep-groups so podman has access to gpu #659
Conversation
Reviewer's Guide by SourceryThis pull request introduces two new command-line options: Sequence diagram for GPU layer configuration and container setupsequenceDiagram
participant User
participant CLI
participant Model
participant Container
User->>CLI: ramalama --keep-groups --ngl 34
CLI->>Model: setup_container(args)
alt Using podman
Model->>Container: Add --group-add keep-groups
end
Model->>Model: gpu_args(args)
Note over Model: Configure GPU layers
Model->>Container: Execute with specified ngl value
Container-->>User: Run model with GPU configuration
Class diagram showing updated Model class with GPU configurationclassDiagram
class Model {
+setup_container(args)
+gpu_args(args, runner)
+build_exec_args_perplexity(args, model_path)
+build_exec_args_bench(args, model_path)
+build_exec_args_run(args, model_path, prompt)
+handle_runtime(args, exec_args, exec_model_path)
}
note for Model "Modified gpu_args to use args.ngl
Updated container setup for GPU access"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @khumarahn - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation explaining the security implications of --keep-groups and when users should/shouldn't use this option
- Consider splitting this PR into two separate ones: one for --ngl and another for the security-sensitive --keep-groups feature
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.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @khumarahn - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add input validation for the --ngl parameter to ensure it's a valid positive integer.
- The --keep-groups option needs more detailed documentation about its security implications and when it should/shouldn't be used.
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.
khumarahn
changed the title
khumarahn
changed the title
|
I added the docs, I think this is ready for a review @sourcery-ai review |
|
@khumarahn changes look fine, CI failed around documentation man pages. You basically need to keep running this: hack/xref-helpmsgs-manpages and change things until it agrees you've done it right. |
| @@ -109,6 +109,13 @@ The default can be overridden in the ramalama.conf file or via the the | |||
| RAMALAMA_IMAGE environment variable. `export RAMALAMA_TRANSPORT=quay.io/ramalama/aiimage:latest` tells | |||
| RamaLama to use the `quay.io/ramalama/aiimage:latest` image. | |||
|
|
|||
| #### **--keep-groups** | |||
There was a problem hiding this comment.
I would prefer to have RamaLama figure this out. or always set it when Podman is used in rootless mode.
Users are not going to understand when and when not to use this.
There was a problem hiding this comment.
I think this should be a decision of maintainers...
I think that "keep-groups" should only be enabled when necessary, and I can't think of a bulletproof logic to determine when it is necessary. So keeping it as an option, off by default, seemed sensible.
There was a problem hiding this comment.
The downside of using it all of the time is a leak of GIDs into the container when not necessary. Add option to allow users to disable it for security purposes, But I think we should enable it by default for rootless containers so that users will not stumble upon it.
ramalama/cli.py
Outdated
| parser.add_argument( | ||
| "--keep-groups", | ||
| dest="podman_keep_groups", | ||
| default=False, |
There was a problem hiding this comment.
Add function to detect, also option should be configurable in ramalama.conf file.
There was a problem hiding this comment.
What is the right way to detect a rootless podman? I found how to detect podman by os.path.basename(args.engine) == "podman", but I'm not sure about rootless
|
There is something wrong with ubi9 images. |
|
Please squash your commits. |
|
@rhatdan I added the options to ramalama.conf. I don't know how to detect if podman is running rootless to turn on keep-groups. Also, I'm not sure this is needed: it goes against the least privilege principle. And turning on this option would be the easiest thing to do for people running gpu in containers (e.g. with nvidia one has to first set up their container toolkit, and keep it updated with the drivers update) |
Signed-off-by: Alexey Korepanov <[email protected]>
done |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @khumarahn - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add more detailed documentation about the security implications of using --keep-groups. What specific permissions are being granted and what are the potential risks?
- Consider adding validation to ensure the --ngl parameter is a positive integer
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Security: 2 issues found
- 🟢 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.
| #### **--keep-groups** | ||
| pass --group-add keep-groups to podman (default: False) | ||
| Needed to access the gpu on some systems, but has an impact on security, use with caution. |
There was a problem hiding this comment.
🚨 suggestion (security): Elaborate on the security implications of --keep-groups.
What are the specific security implications of using this flag?
| #### **--keep-groups** | |
| pass --group-add keep-groups to podman (default: False) | |
| Needed to access the gpu on some systems, but has an impact on security, use with caution. | |
| #### **--keep-groups** | |
| pass --group-add keep-groups to podman (default: False) | |
| Needed to access the gpu on some systems, but has significant security implications: | |
| - Preserves the host user's supplementary group memberships inside the container | |
| - Could grant the container unnecessary elevated privileges through group memberships (e.g., disk, docker, sudo) | |
| - May allow container processes to access host system resources that share the same group permissions | |
| - Breaks container isolation principles by sharing host's security context | |
| Only use this flag if GPU access cannot be achieved through more secure methods like specific device mapping. |
There was a problem hiding this comment.
I think these are great suggestions for documentation at least, dunno if you saw this bot comment @rhatdan @khumarahn
|
LGTM |
| gpu_args = [] | ||
| if ( | ||
| force | ||
| args.gpu |
There was a problem hiding this comment.
I don't think this PR is correct, on systems with integrated graphics we don't want to use GPU acceleration, it's typically much slower than CPU inferencing, practically unusable.
With the default being a static 999 this will always be the case now, which is not good. We should look at this again before we spin up a new release.
There was a problem hiding this comment.
We can have this option but the default being 999 is not good
There was a problem hiding this comment.
I don't think I changed any existing logic here, I just added two options that allow ramalama to work on my system. The default of 999 layers is of course questionable
There was a problem hiding this comment.
It's the lack of VRAM on integrated graphics also, most of the time one will crash on one of these systems because of running out of VRAM
There was a problem hiding this comment.
Sure, but somehow ramalama works on my laptop with no gpu without any configuration. This PR did not change the default behaviour of ramalama, unless I'm missing something
There was a problem hiding this comment.
We should try and figure out why 999 was or wasn't included on your system
Hi. This follows an invitation to submit a PR in #655
I do not feel qualified to submit PRs to ramalama, but I tried to keep it as simple as possible, and to not change any defaults.
With these changes the following works on my system (gentoo linux, an ancient 1080ti with just 11gb of GPU memory):
I didn't touch the documentation yet, hope to get feedback first.
Summary by Sourcery
Add two new command-line options:
--nglto specify the number of GPU layers to use and--keep-groupsto give podman access to the GPU.New Features:
--ngloption to control the number of GPU layers used by the model.--keep-groupsoption for podman to enable GPU access.Summary by Sourcery
Add support for specifying the number of GPU layers to offload using the --ngl option, and allow keeping user groups when using podman with the --keep-groups flag.
New Features:
--ngloption to specify the number of GPU layers to use.--keep-groupsoption to pass--group-add keep-groupsto podman when launching the container. This is useful for enabling GPU access in some systems, but has security implications.