Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
|
/lgtm |
InftyAI-Agent
added
needs-triage
InftyAI-Agent
removed
the
do-not-merge/needs-kind
|
/lgtm |
InftyAI-Agent
added
lgtm
There was a problem hiding this comment.
Pull request overview
This PR refactors model metadata handling by replacing the prior “architecture” concept with a richer ModelSpec, and updates HuggingFace downloads + CLI inspection output to use the new metadata shape.
Changes:
- Replace
ModelArchitecture/archwithModelSpec/specin the model registry. - Fetch model metadata from the HuggingFace REST API during download and store it in the registry.
- Update the
inspectcommand output and associated tests to displayspecfields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/registry/model_registry.rs |
Introduces ModelSpec and updates ModelInfo to persist spec instead of arch; removes old config-based architecture extraction + related tests. |
src/downloader/huggingface.rs |
Adds a HuggingFace REST API metadata fetch and stores the result as ModelSpec; uses API storage size when available. |
src/cli/commands.rs |
Updates inspect output formatting and tests to read/display the new spec fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let spec = Some(ModelSpec { | ||
| author: author_from_api, | ||
| task: task_from_api, | ||
| license: license_from_api, | ||
| model_type: model_type_from_api, | ||
| parameters: parameters_from_api, | ||
| context_window, | ||
| }); |
There was a problem hiding this comment.
spec is always set to Some(ModelSpec { ... }) even when all fields are None, which will serialize into "spec": {} (since the inner fields are skipped). If the intent is “no metadata”, consider setting spec to None when all extracted fields are missing to avoid storing an empty object and to keep the registry format cleaner.
| let spec = Some(ModelSpec { | |
| author: author_from_api, | |
| task: task_from_api, | |
| license: license_from_api, | |
| model_type: model_type_from_api, | |
| parameters: parameters_from_api, | |
| context_window, | |
| }); | |
| let model_spec = ModelSpec { | |
| author: author_from_api, | |
| task: task_from_api, | |
| license: license_from_api, | |
| model_type: model_type_from_api, | |
| parameters: parameters_from_api, | |
| context_window, | |
| }; | |
| let spec = if model_spec.author.is_some() | |
| || model_spec.task.is_some() | |
| || model_spec.license.is_some() | |
| || model_spec.model_type.is_some() | |
| || model_spec.parameters.is_some() | |
| || model_spec.context_window.is_some() | |
| { | |
| Some(model_spec) | |
| } else { | |
| None | |
| }; |
| spec.author.as_deref().unwrap_or("N/A") | ||
| ); | ||
| println!( | ||
| " Task: {}", |
There was a problem hiding this comment.
Context Window is formatted with format_parameters, which produces abbreviated K/M/B output (e.g., 2048 -> "2.05K"). Context window is a token count and should generally be shown as an exact integer (or with a dedicated formatter) to avoid confusing output.
| @@ -102,7 +31,7 @@ pub struct ModelInfo { | |||
| pub updated_at: String, | |||
| pub cache_path: String, | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
There was a problem hiding this comment.
Renaming the persisted field from arch to spec means existing models.json entries that already contain arch will deserialize successfully but the metadata will be silently dropped (since arch is now an unknown field). If preserving existing registry metadata across upgrades matters, consider supporting deserialization of the legacy arch shape (e.g., a LegacyModelArchitecture used only for Deserialize) and mapping it into spec, or implementing a custom Deserialize for ModelInfo that accepts either field.
| #[serde(skip_serializing_if = "Option::is_none")] | |
| #[serde(skip_serializing_if = "Option::is_none", alias = "arch")] |
| let url = format!("https://huggingface.co/api/models/{}", model_name); | ||
| let client = reqwest::Client::new(); | ||
|
|
||
| match client.get(&url).send().await { | ||
| Ok(response) => { | ||
| if let Ok(json) = response.json::<serde_json::Value>().await { | ||
| let author = json |
There was a problem hiding this comment.
fetch_metadata_from_api uses a default reqwest::Client with no timeout and it ignores non-success HTTP statuses. This can cause pull to hang indefinitely on network stalls and silently treat 4xx/5xx responses as “no metadata”. Consider using Client::builder().timeout(...) (or per-request timeout) and calling response.error_for_status() before attempting to parse JSON, returning/logging an error when the API call fails.
| let url = format!("https://huggingface.co/api/models/{}", model_name); | ||
| let client = reqwest::Client::new(); | ||
|
|
||
| match client.get(&url).send().await { |
There was a problem hiding this comment.
The HuggingFace API URL is built via string concatenation with model_name, but model_name may contain characters that require URL encoding. hf_hub likely handles this internally for downloads, so this extra request can fail even when the snapshot download succeeds. Consider constructing the URL with reqwest::Url (or similar) to ensure proper escaping of the repo id/path segment.
| let url = format!("https://huggingface.co/api/models/{}", model_name); | |
| let client = reqwest::Client::new(); | |
| match client.get(&url).send().await { | |
| let mut url = match reqwest::Url::parse("https://huggingface.co/") { | |
| Ok(url) => url, | |
| Err(_) => return (None, None, None, None, None, None), | |
| }; | |
| if let Ok(mut path_segments) = url.path_segments_mut() { | |
| path_segments.push("api"); | |
| path_segments.push("models"); | |
| path_segments.push(model_name); | |
| } else { | |
| return (None, None, None, None, None, None); | |
| } | |
| let client = reqwest::Client::new(); | |
| match client.get(url).send().await { |
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #
Special notes for your reviewer
Does this PR introduce a user-facing change?