-
Notifications
You must be signed in to change notification settings - Fork 48
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
Failover to OCI when push fails with default push mechanism #476
Conversation
Signed-off-by: Daniel J Walsh <[email protected]>
Reviewer's Guide by SourceryThis PR implements a failover mechanism in the push operation. When the default push mechanism fails with a KeyError, the system attempts to push the model as a container image using OCI (Open Container Initiative) format. This provides a fallback option for handling push operations when the primary method fails. Sequence diagram for failover mechanism in push operationsequenceDiagram
participant User
participant CLI
participant NewModel
participant OCIModel
User->>CLI: push_cli(args)
CLI->>NewModel: New(tgt, args)
NewModel->>NewModel: push(source, args)
alt Push succeeds
NewModel-->>CLI: Success
else Push fails with KeyError
CLI->>OCIModel: OCI(model, config)
OCIModel->>OCIModel: push(args)
alt OCI Push succeeds
OCIModel-->>CLI: Success
else OCI Push fails
OCIModel-->>CLI: Raise KeyError
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 adding logging when falling back to OCI push to make it clear to users when this occurs
- The error handling could be improved - catching a broad Exception and re-raising the original KeyError could mask important errors. Consider handling specific exceptions and providing more informative error messages
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
if model.startswith(mtype + "://"): | ||
raise e | ||
try: | ||
# attempt to push as a container 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.
suggestion: Consider adding logging or user feedback when falling back to container image handling for better debugging experience.
# attempt to push as a container image | |
logger.info("Falling back to container image handling for model push") | |
# attempt to push as a container image |
Summary by Sourcery
Enhancements: