-
Notifications
You must be signed in to change notification settings - Fork 234
Don't pull image when doing ramalama --help call #1589
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
Fixes: containers#1587 Signed-off-by: Daniel J Walsh <[email protected]>
Reviewer's GuideThis PR prevents image pulls during the Sequence diagram for CLI argument parsing with nopull flagsequenceDiagram
actor User
participant CLI as ramalama.cli
participant AccelImage as accel_image
User->>CLI: ramalama --help
CLI->>AccelImage: accel_image(CONFIG, nopull=True)
AccelImage-->>CLI: image string (no pull)
CLI-->>User: Help output
Class diagram for updated accel_image function signature and usageclassDiagram
class accel_image {
+str accel_image(config: Config, nopull=False)
}
class Config
ramalama.cli --|> accel_image : uses
ramalama.cli : --image default=accel_image(CONFIG, nopull=True)
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.
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!
I've implemented a change to prevent the ramalama
CLI from attempting to pull container images when the --help
command is executed. This improves the responsiveness of the help command by avoiding unnecessary network operations, addressing a reported issue where ramalama --help
would trigger an image pull.
Highlights
- Prevent Image Pull on Help: I've modified the
ramalama
CLI's--image
argument default to pass anopull=True
flag to theaccel_image
function. This ensures that image pulling is bypassed when--help
is invoked, improving the responsiveness of the command. - Introduce
nopull
Parameter: I've added a newnopull
boolean parameter to theaccel_image
function inramalama/common.py
. This parameter, defaulting toFalse
, allows callers to explicitly prevent image pull attempts. - Conditional Image Versioning: I've updated the
accel_image
logic to short-circuit theattempt_to_use_versioned
call if thenopull
flag is set. This effectively prevents network operations for image checks whennopull
is active.
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. ↩
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.
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.
Code Review
This pull request effectively resolves the issue of ramalama --help
triggering an unnecessary image pull. The approach of adding a nopull
flag to accel_image
is clean and well-targeted. The changes in both cli.py
and common.py
are correct and work together to implement the fix.
My main feedback is to enhance test coverage for the new logic path to ensure its robustness and prevent future regressions. I've added a comment with details on this.
Overall, this is a solid contribution that improves the user experience.
@@ -650,7 +650,7 @@ def accel_image(config: Config) -> str: | |||
return "registry.redhat.io/rhelai1/ramalama-vllm" | |||
|
|||
vers = minor_release() | |||
if attempt_to_use_versioned(config.engine, image, vers, True): | |||
if nopull or attempt_to_use_versioned(config.engine, image, vers, True): |
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.
The new logic path introduced by the nopull
flag is not covered by existing unit tests. To ensure the correctness of this change and prevent future regressions, please consider adding a test case to test/unit/test_common.py
that verifies the behavior when nopull=True
.
The test should:
- Call
accel_image(config, nopull=True)
. - Assert that the returned image name has the correct version tag.
- Assert that
attempt_to_use_versioned
is not called, for example by usingmock.assert_not_called()
.
Thanks Eric & Dan! I proposed a small improvement here: #1611 |
Fixes: #1587
Summary by Sourcery
Add a
nopull
option to the image selection logic and apply it in the CLI so that runningramalama --help
(or other non-runtime invocations) no longer triggers a container image pull.Bug Fixes:
ramalama --help
Enhancements:
nopull
parameter toaccel_image
to bypass pulling the container imageaccel_image
withnopull=True
when setting the CLI’s default--image
value