-
Notifications
You must be signed in to change notification settings - Fork 117
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
Fix up handling of image selection on generate #856
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces fallback logic to use OCI images when the initial attempt to load a model fails. It also modifies the Updated class diagram for QuadletclassDiagram
class Quadlet {
-ai_image: str
-model: str
-args: dict
-exec_args: list
-image: str
+__init__(model: str, image: str, args: dict, exec_args: list)
+kube()
+generate()
}
note for Quadlet "The image parameter was added to the constructor."
Updated class diagram for KubeclassDiagram
class Kube {
-ai_image: str
-model: str
-args: dict
-exec_args: list
-image: str
+__init__(model: str, image: str, args: dict, exec_args: list)
+gen_volumes()
}
note for Kube "The image parameter was added to the constructor."
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.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the duplicated try-except blocks in
run_cli
andserve_cli
into a single function. - It looks like the
image
attribute is being passed around to multiple classes; consider if there's a better way to manage this shared state.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 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.
except Exception: | ||
raise e |
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 (bug_risk): Re-raising the original KeyError in the fallback may mask the actual OCI error.
If the OCI branch fails, its underlying error will be lost. Consider chaining the exceptions or re-raising the OCI error to better surface the true cause.
except Exception: | |
raise e | |
except Exception as oci_error: | |
raise oci_error from e |
test/system/055-convert.bats
Outdated
run_ramalama serve -t ${cname} -d tiny ramalama/tiny | ||
run_ramalama stop -t ${cname} |
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 (testing): Consider adding tests for the fallback behavior.
Since the PR description mentions a fallback to OCI images, it's important to verify this behavior with dedicated tests. Try serving/running a model that doesn't exist as a New
model, but does exist as an OCI image, and confirm the fallback logic works as expected. This would involve testing the try...except
blocks in run_cli
and serve_cli
.
95364d4
to
a706567
Compare
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.
Haven't looked at why the tests are failing or anything but the code looks fine
Also fall back to trying OCI images on ramalama run and serve. Signed-off-by: Daniel J Walsh <[email protected]>
The issue is an older version of podman, I believe in Ubuntu. |
There's a super hacky "Upgrade to podman 5" stage in at least some of our builds. I think it was installing Ubuntu 24.10 podman on Ubuntu 24.04 |
Also fall back to trying OCI images on ramalama run and serve.
Summary by Sourcery
This pull request modifies the application to improve image selection and handling, particularly when using
run
andserve
commands. It introduces a fallback mechanism to attempt using OCI images if the initial image type fails. Additionally, it refactors theQuadlet
andKube
classes to receive the image as a parameter, ensuring correct image usage in container configurations.Bug Fixes:
run
andserve
commands.Enhancements:
Quadlet
andKube
classes to receive the image as a parameter during initialization, ensuring the correct image is used for container configuration.