-
Notifications
You must be signed in to change notification settings - Fork 236
Don't override image when using rag if user specified it #1727
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 GuideAdds logic to respect a new image_override flag so that user-specified images are not automatically suffixed with -rag, adjusts both the generate and container name routines to use this flag, and includes a system test to verify the behavior. 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!
This pull request addresses an issue where the ramalama
tool would unintentionally override a user-specified container image with its RAG-specific variant, even when the user had explicitly provided an image. The changes ensure that if a user provides a custom image and indicates it should not be overridden, the tool respects that choice, improving user control and predictability.
Highlights
- Preventing Image Override for RAG: Modified the logic in
ramalama/model.py
andramalama/rag.py
to prevent the automatic transformation of a user-specified image into its-rag
variant when therag
feature is enabled, provided animage_override
flag is set. Previously, therag
image was forced if the default image was used andrag
was enabled. Now, therag
image is only applied ifrag
is enabled ANDimage_override
is NOT true, ensuring user's explicit image choice is respected. - New Test Case for Image Override: Added a new system test in
test/system/070-rag.bats
to verify that when a specific--image
is provided along with--rag
, the tool does not append-rag
to the user-specified image, confirming the intended behavior of respecting the user's image choice.
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 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 or fill out our survey 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
The pull request prevents overriding a user-specified image when using the RAG feature. A critical regression was identified in ramalama/model.py
where the default RAG image was not being used when a custom image wasn't specified. The default value for getattr
should be False
to fix this. Also, the new system test was enhanced to cover all scenarios.
ramalama/model.py
Outdated
@@ -258,7 +258,7 @@ def get_container_name(self, args): | |||
def base(self, args, name): | |||
# force accel_image to use -rag version. Drop TAG if it exists | |||
# so that accel_image will add -rag to the image specification. | |||
if args.image == self.default_image and getattr(args, "rag", None): | |||
if getattr(args, "rag", None) and not getattr(args, "image_override", 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.
This change introduces a regression. While it correctly prevents overriding a user-specified image, it breaks the case where --rag
is used without a custom --image
.
In that scenario, args.image_override
is not set, so getattr(args, "image_override", True)
returns the default value True
. This makes the condition not getattr(...)
evaluate to False
, and the -rag
suffix is not appended to the default image. This breaks existing correct behavior.
The default value for getattr
should be False
to fix this. This would also make it consistent with the logic in ramalama/rag.py
.
With False
as the default:
- No
--image
:getattr
returnsFalse
,not False
isTrue
, condition passes,-rag
suffix is added. (Correct) - With
--image
:image_override
isTrue
,getattr
returnsTrue
,not True
isFalse
, condition fails, user image is respected. (Correct)
if getattr(args, "rag", None) and not getattr(args, "image_override", True): | |
if getattr(args, "rag", None) and not getattr(args, "image_override", False): |
run_ramalama --image quay.io/ramalama/bogus --dryrun run --rag quay.io/ramalama/myrag:1.2 ollama://smollm:135m | ||
assert "$output" !~ ".*quay.io/ramalama/bogus-rag.*" "Expected to not use -rag image" |
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.
This test verifies that a user-provided image is not overridden. Consider adding a test case for the opposite scenario: when --rag
is used without a custom --image
, the default image is correctly modified to use the -rag
version.
A complete test suite for this logic would cover both cases, ensuring both the fix and existing functionality are preserved.
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.
A previous check already handles this.
Fixes: containers#1713 Signed-off-by: Daniel J Walsh <[email protected]>
LGTM |
/packit retest-failed |
Fixes: #1713
Summary by Sourcery
Respect user-provided images by applying the RAG suffix only when the image_override flag is not set, and add a system test to validate this behavior
Bug Fixes:
Enhancements:
Tests: