-
Notifications
You must be signed in to change notification settings - Fork 235
Snapshot verification #1458
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
Snapshot verification #1458
Conversation
Reviewer's GuideIntroduces a dedicated snapshot verification step for model endianness, centralizes GGUF parsing logic and system endianness helpers, refactors snapshot creation with robust error handling and automatic cleanup, and removes redundant code and parameters across model store and client wrappers. Sequence Diagram for Updated
|
Change | Details | Files |
---|---|---|
Introduce dedicated snapshot endianness verification step |
|
ramalama/model_store.py |
Centralize GGUF endianness determination and system helper functions |
|
ramalama/gguf_parser.py ramalama/endian.py |
Refactor new_snapshot to wrap steps in error-handling and ensure cleanup |
|
ramalama/model_store.py |
Improve error handling in remove_snapshot |
|
ramalama/model_store.py |
Eliminate redundant endianness flags and simplify client wrappers |
|
ramalama/model_store.py ramalama/repo_model_base.py ramalama/ollama.py |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai review
on the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issue
to create an issue from it. - Generate a pull request title: Write
@sourcery-ai
anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title
on the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summary
anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summary
on the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guide
on the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolve
on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismiss
on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review
to trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
Hi, when this PR is ready, please allow me to test it on IBM Z & LinuxONE as well to ensure that the endianness checks are still intact :) Self-assigning for now |
@taronaeo Ping :) |
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 @engelmi - I've reviewed your changes - here's some feedback:
- In
_verify_endianess
, replace theif ref_file is None: pass
with an explicit earlyreturn
to make the no-ref_file case clearer and prevent accidental fallthrough. - The generic
except Exception
block innew_snapshot
can mask unexpected bugs—consider catching only the specific errors you expect or re-raising after cleanup to preserve the original traceback. - You have almost identical cleanup logic in both error handlers in
new_snapshot
—extracting that into a small helper function could reduce duplication and make the flow clearer.
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.
Gotcha! I have dispatched a runner to test this PR, will update again once the test is complete :) |
2db17bb
to
322d65b
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.
LGTM! Works as intended on IBM Z & LinuxONE!
Signed-off-by: Michael Engel <[email protected]>
By moving the recently improved code to detect the endianness into a dedicated function, its reusability is increased. Also, a specific exception class if the model is not in the gguf format has been added. Signed-off-by: Michael Engel <[email protected]>
Previously, the endianness check was done for each SnapshotFile and these files might not be models, but could also be miscellaneous such as chat templates or other meta data. By removing only the affected file on a mismatch error the store might get into an inconsistent state since the cleanup depends on the error handling of the caller. Therefore, the check for endianness has been moved one layer up and only checks the flagged model file. In case of a mismatch an implicit removal of the whole snapshot is triggered. Signed-off-by: Michael Engel <[email protected]>
An error when creating new snapshots has only been partially handled inside the model store and the caller side had to clean up properly. In order to simplify this, more error handling has been added when creating new snapshots - removing the (faulty) snapshot, logging and passing the exception upwards so that the caller can do additional actions. This ensures that the state remains consistent. Signed-off-by: Michael Engel <[email protected]>
Signed-off-by: Michael Engel <[email protected]>
322d65b
to
b84527b
Compare
Summary by Sourcery
Implement snapshot endianness verification, refactor GGUF parsing for clearer endianness detection, and improve robustness of snapshot creation and cleanup flows.
New Features:
Enhancements: