-
Notifications
You must be signed in to change notification settings - Fork 49
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
Attempt to remove OCI Image if removing as Ollama or Huggingface fails #432
Conversation
Signed-off-by: Daniel J Walsh <[email protected]>
Reviewer's Guide by SourceryThis PR enhances the model removal functionality by adding a fallback mechanism. If removing a model as an Ollama or Huggingface model fails, the system will attempt to remove it as an OCI container image before raising the original error. Sequence diagram for model removal processsequenceDiagram
participant User
participant CLI as Ramalama CLI
participant Model as Model
participant OCI as OCI Image
User->>CLI: Request to remove model
CLI->>Model: Attempt to remove as Ollama/Huggingface
alt Removal successful
Model-->>CLI: Model removed
else Removal fails
CLI->>OCI: Attempt to remove as OCI Image
alt OCI removal successful
OCI-->>CLI: OCI Image removed
else OCI removal fails
CLI-->>User: Raise original error
end
end
File-Level Changes
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 improving error handling to preserve the OCI removal error instead of re-raising the original KeyError. This would make debugging easier when the fallback fails.
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.
# attempt to remove as a container image | ||
m=OCI(model, config.get('engine', container_manager())) | ||
m.remove(args) | ||
except Exception: |
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): Consider preserving the OCI exception instead of re-raising the KeyError
When the OCI operation fails, re-raising the original KeyError could mask important error information. Consider either letting the OCI exception propagate or wrapping it with additional context.
except Exception as oci_error:
raise oci_error from e
@@ -89,7 +89,7 @@ load setup_suite | |||
|
|||
run_ramalama rm oci://$registry/tiny | |||
run_ramalama rm oci://$registry/tiny-car | |||
run_ramalama rm oci://$registry/mymodel | |||
run_ramalama rm $registry/mymodel |
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.
issue (testing): Test coverage for new error handling path is missing
The PR adds new error handling logic to attempt OCI image removal as a fallback, but there are no tests verifying this behavior. Consider adding test cases that verify: 1) successful fallback to OCI removal, 2) proper error propagation when both attempts fail, and 3) verification that the original error is raised when OCI removal also fails.
Summary by Sourcery
Implement a fallback mechanism to remove a model as an OCI Image if removal as Ollama or Huggingface fails, and update the system test accordingly.
Bug Fixes:
Tests: