move --image & --keep-groups to run, serve, perplexity, bench commands #1669
Conversation
rhatdan
requested review from
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver and
engelmi
as code owners
Reviewer's GuideThis PR confines the "--image" and "--keep-groups" options to the container-specific commands (run, serve, perplexity, bench) instead of the global CLI, updates the info command to always resolve the image from configuration, adjusts documentation accordingly, and revises system tests to use the RAMALAMA_IMAGE environment variable for image selection. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rhatdan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the command-line interface for the ramalama tool by scoping container-related options to only the commands that utilize containers. This change improves the clarity and usability of the CLI by preventing users from accidentally interacting with container options when not running containerized workloads, and it cleans up the main command's help output.
Highlights
- CLI Argument Relocation: I've moved the
--imageand--keep-groupscommand-line options from the mainramalamacommand to specific subcommands:run,serve,perplexity, andbench. This ensures these container-specific options are only available where they are relevant. - Documentation Updates: I've updated the man pages (
.1.mdfiles) forramalama-bench,ramalama-perplexity,ramalama-run, andramalama-serveto include the documentation for the newly added--imageand--keep-groupsoptions. Conversely, I've removed their documentation from the generalramalama.1.mdman page. - Internal Argument Parsing Logic: In
ramalama/cli.py, I've adjusted the argument parsing. The--imageand--keep-groupsarguments are now suppressed from the global parser and are instead added dynamically to the parsers for therun,serve,perplexity, andbenchcommands via theruntime_optionsfunction. Theinfo_clifunction now directly uses theaccel_image(CONFIG)for image information, reflecting that--imageis no longer a global argument. - Test Suite Adjustments: I've updated the system tests in
test/system/060-info.batsto reflect these changes. Specifically, the test for the 'Image' field now uses a more general regex, and the image is passed via theRAMALAMA_IMAGEenvironment variable instead of the--imageCLI flag for theinfocommand, aligning with the new argument structure.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
- Consider emitting a deprecation warning for the removed global --image/--keep-groups flags rather than silently suppressing them to smooth user migration.
- The repeated man-page blocks for --image and --keep-groups are duplicated across several docs—consider centralizing or templating that text to avoid drift.
- Info no longer honors an explicit --image override; you may want to restore support for user-specified images in info rather than always using accel_image(CONFIG).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider emitting a deprecation warning for the removed global --image/--keep-groups flags rather than silently suppressing them to smooth user migration.
- The repeated man-page blocks for --image and --keep-groups are duplicated across several docs—consider centralizing or templating that text to avoid drift.
- Info no longer honors an explicit --image override; you may want to restore support for user-specified images in info rather than always using accel_image(CONFIG).
## Individual Comments
### Comment 1
<location> `docs/ramalama-bench.1.md:52` </location>
<code_context>
show this help message and exit
+#### **--image**=IMAGE
+OCI container image to run with specified AI model. RamaLama defaults to use
+images based on the accelerator it discovers. For example:
+`quay.io/ramalama/ramalama`. See the table below for all default images.
+The default image tag is based on the minor version of the RamaLama package.
</code_context>
<issue_to_address>
Consider rephrasing 'defaults to use images' for grammatical correctness.
Change to 'RamaLama defaults to using images' or 'RamaLama defaults to images' for better grammar.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
OCI container image to run with specified AI model. RamaLama defaults to use
images based on the accelerator it discovers. For example:
=======
OCI container image to run with specified AI model. RamaLama defaults to using
images based on the accelerator it discovers. For example:
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `docs/ramalama-perplexity.1.md:57` </location>
<code_context>
show this help message and exit
+#### **--image**=IMAGE
+OCI container image to run with specified AI model. RamaLama defaults to use
+images based on the accelerator it discovers. For example:
+`quay.io/ramalama/ramalama`. See the table below for all default images.
+The default image tag is based on the minor version of the RamaLama package.
</code_context>
<issue_to_address>
Consider rephrasing 'defaults to use images' for grammatical correctness.
Change to 'RamaLama defaults to using images' or 'RamaLama defaults to images' for better grammar.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
OCI container image to run with specified AI model. RamaLama defaults to use
images based on the accelerator it discovers. For example:
=======
OCI container image to run with specified AI model. RamaLama defaults to using
images based on the accelerator it discovers. For example:
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
docs/ramalama-bench.1.md
Outdated
| OCI container image to run with specified AI model. RamaLama defaults to use | ||
| images based on the accelerator it discovers. For example: |
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing 'defaults to use images' for grammatical correctness.
Change to 'RamaLama defaults to using images' or 'RamaLama defaults to images' for better grammar.
| OCI container image to run with specified AI model. RamaLama defaults to use | |
| images based on the accelerator it discovers. For example: | |
| OCI container image to run with specified AI model. RamaLama defaults to using | |
| images based on the accelerator it discovers. For example: |
docs/ramalama-perplexity.1.md
Outdated
| OCI container image to run with specified AI model. RamaLama defaults to use | ||
| images based on the accelerator it discovers. For example: |
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing 'defaults to use images' for grammatical correctness.
Change to 'RamaLama defaults to using images' or 'RamaLama defaults to images' for better grammar.
| OCI container image to run with specified AI model. RamaLama defaults to use | |
| images based on the accelerator it discovers. For example: | |
| OCI container image to run with specified AI model. RamaLama defaults to using | |
| images based on the accelerator it discovers. For example: |
There was a problem hiding this comment.
Code Review
This pull request moves the --image and --keep-groups flags from global options to the run, serve, perplexity, and bench subcommands, which is a good refactoring to make the CLI more logical. The implementation looks correct, including updates to tests and documentation. I've found a minor formatting issue in a help string and a copy-paste error in the documentation that should be addressed.
docs/ramalama-bench.1.md
Outdated
| OCI container image to run with specified AI model. RamaLama defaults to use | ||
| images based on the accelerator it discovers. For example: | ||
| `quay.io/ramalama/ramalama`. See the table below for all default images. | ||
| The default image tag is based on the minor version of the RamaLama package. | ||
| Version 0.10.0 of RamaLama pulls $IMAGE:0.10 from the quay.io/ramalama OCI repository. The --image option overrides this default. |
There was a problem hiding this comment.
This documentation for --image appears to be copied from another file and has a couple of issues that could confuse users:
- It mentions "See the table below for all default images", but there is no table in this file. This reference is broken. It would be better to refer to the
ramalama(1)man page where the table is located. - The sentence "Version 0.10.0 of RamaLama pulls $IMAGE:0.10..." is unclear because
$IMAGEis not a defined variable in this context.
This same issue is present in the other new documentation files: docs/ramalama-perplexity.1.md, docs/ramalama-run.1.md, and docs/ramalama-serve.1.md.
| OCI container image to run with specified AI model. RamaLama defaults to use | |
| images based on the accelerator it discovers. For example: | |
| `quay.io/ramalama/ramalama`. See the table below for all default images. | |
| The default image tag is based on the minor version of the RamaLama package. | |
| Version 0.10.0 of RamaLama pulls $IMAGE:0.10 from the quay.io/ramalama OCI repository. The --image option overrides this default. | |
| OCI container image to run with specified AI model. RamaLama defaults to use | |
| images based on the accelerator it discovers. For example: | |
| `quay.io/ramalama/ramalama`. See `ramalama(1)` for a table of all default images. | |
| The default image tag is based on the minor version of the RamaLama package. | |
| Version 0.10.0 of RamaLama pulls an image with a `:0.10` tag from the quay.io/ramalama OCI repository. The --image option overrides this default. |
ramalama/cli.py
Outdated
| help="""pass `--group-add keep-groups` to podman, if using podman. | ||
| Needed to access gpu on some systems, but has security implications.""", |
There was a problem hiding this comment.
The help text for --keep-groups has incorrect indentation on the second line. This will cause the help message displayed to the user to have extra leading spaces, which can look unprofessional. The second line of the help string should not be indented relative to the start of the string content on the first line.
| help="""pass `--group-add keep-groups` to podman, if using podman. | |
| Needed to access gpu on some systems, but has security implications.""", | |
| help="""pass `--group-add keep-groups` to podman, if using podman. | |
| Needed to access gpu on some systems, but has security implications.""", |
|
This is a possible breaking change, which is why I kept the hidden --image option on ramalama, I don't think it is worth having a --keep-groups hidden option, since I don't believe many people use it or know about it. Since these options are only used for container operations, makes little sense to keep them global. So moving them specifically to container operations eliminates the side effect of us pulling an image for commands like ramalama rm, pull rm ... We could also move the --container and --nocontainer options, and perhaps should. |
rhatdan
force-pushed
the
image
branch
4 times, most recently
from
dc2df27 to
384b958
Compare
This eliminates the need for pulling images by accident when not using containers. Since these commands are only used for container commands, no need for them in other places. Fixes: containers#1662 Signed-off-by: Daniel J Walsh <[email protected]>
|
LGTM! Once bat tests are done can merge! |
|
Tested works! |
This eliminates the need for pulling images by accident when not using containers. Since these commands are only used for container commands, no need for them in other places.
Fixes: #1662
Summary by Sourcery
Restrict image and podman keep-groups options to only containerized commands and update related behavior, documentation, and tests.
Bug Fixes:
Enhancements:
Documentation:
Tests: