Fix listing models #1748
Fix listing models #1748
Conversation
engelmi
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars and
cgruver
as code owners
Reviewer's GuideExtract migration logic for converting old RefFile formats into RefJSONFile into a dedicated function and invoke it during get_ref_file and list_models, removing redundant mapping code and relocating filename sanitization. Sequence diagram for listing models with migration checksequenceDiagram
participant GlobalStore
participant FileSystem
participant RefFile
participant RefJSONFile
participant Migration
GlobalStore->>FileSystem: List ref files in ref_dir
loop For each ref_file_name
GlobalStore->>Migration: migrate_reffile_to_refjsonfile(ref_file_path, snapshot_dir)
alt Migration returns RefJSONFile
Migration-->>GlobalStore: RefJSONFile
else Migration returns None
GlobalStore->>RefJSONFile: RefJSONFile.from_path(ref_file_path)
end
GlobalStore->>RefJSONFile: Access files for model listing
end
Class diagram for RefFile and RefJSONFile migration changesclassDiagram
class RefFile {
+from_path(path: str)
+serialize()
+write_to_file()
+map_to_refjsonfile(ref_file_path: str, snapshot_directory: str) RefJSONFile
}
class RefJSONFile {
+from_path(path: str)
+write_to_file()
hash
path
files
}
class StoreFile {
hash
name
type
}
class StoreFileType
RefFile --|> RefJSONFile : migration
RefJSONFile o-- StoreFile : files
StoreFile --> StoreFileType : type
Class diagram for ModelStore and migration integrationclassDiagram
class ModelStore {
refs_directory
snapshots_directory
+get_ref_file_path(model_tag: str) str
+get_ref_file(model_tag: str) RefJSONFile|None
}
class RefJSONFile
class RefFile
ModelStore --> RefJSONFile : uses
ModelStore --> RefFile : uses
ModelStore ..> migrate_reffile_to_refjsonfile : calls
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @engelmi, 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 models were incorrectly listed with zero bytes due to a missing migration step for reference files. It introduces a robust mechanism to detect and automatically migrate older RefFile formats to the new RefJSONFile format when models are being listed or retrieved. This ensures proper model file accounting and prepares for future removal of the old format by centralizing the migration logic into a dedicated function.
Highlights
- RefFile Migration Logic Centralization: The logic for migrating
RefFiletoRefJSONFilehas been extracted into a dedicated, reusable functionmigrate_reffile_to_refjsonfilewithinreffile.py. This function now handles the conversion of old reference file formats to the new JSON-based format, including determining file types and blob hashes. - Automatic Migration during Model Listing: The
list_modelsfunction inglobal_store.pynow proactively checks for and triggers theRefFiletoRefJSONFilemigration when iterating through reference files. This resolves the issue where models were being listed with incorrect sizes (0 bytes) because their associated reference files were in an outdated format. - Filename Sanitization Utility: A new
sanitize_filenameutility function has been added tocommon.py. This function replaces colons with hyphens in filenames, improving consistency and preventing potential issues with file paths. - Consistent RefJSONFile Adoption: The codebase now consistently uses
RefJSONFilefor managing model references. Backward compatibility is ensured by the new migration routine, which is invoked both during model listing and when retrieving individual reference files.
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. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @engelmi - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `ramalama/model_store/reffile.py:104` </location>
<code_context>
+
+ def determine_blob_hash(filename: str) -> str:
+ blob_path = Path(os.path.join(snapshot_directory, sanitize_filename(ref_file.hash), filename)).resolve()
+ if not os.path.exists(blob_path):
+ return generate_sha256(filename)
+ return blob_path.stem
+
</code_context>
<issue_to_address>
Returning generate_sha256(filename) when the file does not exist may be misleading.
Returning a hash for a non-existent file may cause confusion or downstream errors. Consider raising an error or returning None instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@BobbyRadford I think this should fix your opened issue #1748. Could you verify it? |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with listing models by ensuring that legacy RefFile formats are migrated to the new RefJSONFile format on the fly. The changes include refactoring the migration logic into dedicated utility functions, which improves code organization and maintainability.
My review focuses on the new migration logic in ramalama/model_store/reffile.py. I've identified a potential data corruption issue where a hash is generated from a filename for missing files, and I've suggested a safer alternative. I also have a couple of suggestions to improve code quality and robustness by using more specific exception handling and adhering to Python conventions.
Signed-off-by: Michael Engel <[email protected]>
Fixes: containers#1735 The migration for RefFile to RefJSONFile needs to be checked, and optionally triggered, when listing models. Otherwise no files are being found and the model is listed with 0 Bytes. Signed-off-by: Michael Engel <[email protected]>
engelmi
force-pushed
the
fix-listing-models
branch
from
12e8e06 to
d48e404
Compare
|
LGTM |
Fixes: #1735
The migration for RefFile to RefJSONFile needs to be checked, and optionally triggered, when listing models. Otherwise no files are being found and the model is listed with 0 Bytes. It also moves the migrating of RefFile to RefJSONFile into a dedicated function so that it can be removed more easily in later releases (when this is no longer needed).
Summary by Sourcery
Fix model listing by migrating legacy RefFile to the new RefJSONFile format during listing, refactor migration logic into dedicated utilities, and clean up duplicate mapping code.
Bug Fixes:
Enhancements:
Chores: