-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add model inspect cli #776
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new 'show' CLI command that displays model information, including support for both concise and full details (when using the --all and --json options). The changes include enhancements for GGUF file format support, refactoring of the model base class to properly set and retrieve directory and filename information, and cleanup of duplicate implementations in model subclasses. Additionally, new modules for parsing GGUF data and serializing model metadata have been added. Sequence diagram for the 'show' CLI commandsequenceDiagram
actor User as User
participant CLI as Show CLI
participant Model as New(Model)
participant Parser as GGUFInfoParser
participant BaseInfo as ModelInfoBase
User->>CLI: Execute 'ramalama show MODEL [--json] [--all]'
CLI->>Model: Instantiate New(args.MODEL, args)
CLI->>Parser: is_model_gguf(model, args)?
alt GGUF model supported
Parser-->>CLI: true
CLI->>Parser: parse(model, args)
Parser-->>CLI: gguf_info
CLI->>CLI: gguf_info.serialize(json, all)
CLI-->>User: Display full model info output
else Not a GGUF model
Parser-->>CLI: false
CLI->>Model: get filename, get_model_path(args), get_model_registry(args)
CLI->>BaseInfo: Create ModelInfoBase instance
BaseInfo-->>CLI: Instance created
CLI->>BaseInfo: serialize(json)
CLI-->>User: Display base model info output
end
Class diagram for Model, GGUFInfoParser, and ModelInfo classesclassDiagram
class Model {
- model: string
- directory: string
- filename: string
+ __init__(model)
+ get_model_path(args)
+ get_model_registry(args)
+ login(args)
+ build_exec_args_bench(args, model_path)
}
class ModelInfoBase {
+ Name: string
+ Registry: string
+ Path: string
+ serialize(json: bool): string
+ to_json(): string
}
class GGUFModelInfo {
+ Format: string
+ Version: number
+ Metadata: Map
+ Tensors: list
+ LittleEndian: bool
+ serialize(json: bool, all: bool): string
+ to_json(all: bool): string
<<constructor>>
}
class GGUFInfoParser {
<<static>> + is_model_gguf(model, cli_args): bool
<<static>> + parse(model, cli_args): GGUFModelInfo
}
ModelInfoBase <|-- GGUFModelInfo
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 @engelmi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a try-except block around the GGUF parsing logic to handle potential file errors more gracefully.
- The
ModelInfoBase.serialize
method could use a more robust string formatting approach.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
In a follow-up PR we can introduce downloading the metadata in separate files (non-gguf encoded models) such the |
New files must be added to install.sh, looks like our CI didn't catch that here |
Looks like a good idea to me! CI needs to be fixed up |
Ah, forgot about that... let me check if I can fix the CI and the CI check as well. |
We did run this as part of CI in the past, it would have picked up many of these things: https://github.com/containers/ramalama/blob/main/test/ci.sh it's an option to re-add that. Our CI got reworked a few times and it likely slipped through the cracks. The files check was just this grep:
which seems kinda basic, but meh, it worked. Slightly better would be an install and smoke test, just execute something very simple on a properly installed system. |
I'm assuming we considered the:
binaries, some of them might already implement this... I'm ok with a from scratch python3 solution though 😄 |
Haven't thought about this, to be honest. |
SGTM |
98aead8
to
a82bcd6
Compare
dd134d4
to
798c82a
Compare
@engelmi Great job, I really like this feature. BUT: These need tests. We potentially will need to add something like --format option to allow users goformat stuff like podman inspect. |
798c82a
to
d0f036e
Compare
d0f036e
to
c56a632
Compare
On next pass please fix the commit message show->inspect. |
Thanks! |
I would love to start adding unit tests, but for now system tests are fine. I just want you to add a new test/system/100-inspect.bats or something and add a few tests on the tiny models. |
c56a632
to
5d46b1a
Compare
Rename ramalama/model_info.py to ramalama/model_inspect.py |
5d46b1a
to
6020466
Compare
6020466
to
d3b01bf
Compare
66627ee
to
64843f4
Compare
AI Models are shipped with a lot of (meta) information such as the used architecture, the chat template it requires and so on. In order to make these available to the user, the new CLI command inspect with the option support for --all and --json has been implemented. At the moment the GGUF file format - which includes the model as well as the (meta) information in one file - is fully supported. Other formats where the model and information is stored in different files are not (yet) supported and only display basic information such as the model name, path and registry. Signed-off-by: Michael Engel <[email protected]>
The directory and filename of a model is determined by the respective model implementation, e.g. Ollama or Huggingface. If, however, these two fields are not defined in the model base class, then accessing them for a specific model instance might fail since these do not exist. Signed-off-by: Michael Engel <[email protected]>
Signed-off-by: Michael Engel <[email protected]>
Signed-off-by: Michael Engel <[email protected]>
64843f4
to
7f8a046
Compare
Added show CLI command to display model info
AI Models are shipped with a lot of (meta) information such as the used architecture, the chat template it requires and so on. In order to make these available to the user, the new CLI command show with the option support for --all and --json has been implemented.
At the moment the GGUF file format - which includes the model as well as the (meta) information in one file - is fully supported. Other formats where the model and information is stored in different files are not (yet) supported and only display basic information such as the model name, path and
Signed-off-by: Michael Engel [email protected]
Example: smollm:135 (ollama)
Example: granite-7b-lab-Q4_K_M.gguf (huggingface)
Exmaple: smollm:135 with all
Fix: Set directory and filename in Model base class
The directory and filename of a model is determined by the respective model implementation, e.g. Ollama or Huggingface. If, however, these two fields are not defined in the model base class, then accessing them for a specific model instance might fail since these do not exist.
Signed-off-by: Michael Engel [email protected]
Summary by Sourcery
New Features: