Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the model metadata shape by renaming the stored “artifact” metadata to “cache” and adjusts ModelInfo field ordering, updating all call sites (CLI, downloader, registry, and tests) to match.
Changes:
- Rename
ArtifactInfo→CacheInfoandModelMetadata.artifact→ModelMetadata.cacheacross the codebase. - Reorder
ModelInfofields (moveproviderearlier) and update SQLite row-to-struct mapping accordingly. - Update CLI output and unit/integration tests to use the new
cachefield names.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/sqlite.rs | Updates SQLite row mapping to ModelInfo and adjusts storage tests for metadata.cache. |
| src/registry/model_registry.rs | Renames metadata structs/fields (artifact → cache) and updates registry deletion path usage. |
| src/downloader/huggingface.rs | Writes downloaded model metadata using CacheInfo/metadata.cache. |
| src/cli/rm.rs | Updates CLI rm tests to construct models with metadata.cache. |
| src/cli/ls.rs | Updates CLI ls tests to construct models with metadata.cache. |
| src/cli/inspect.rs | Updates inspect display to read revision/size/path from metadata.cache. |
| src/cli/commands.rs | Updates listing logic and CLI command tests to use metadata.cache. |
| src/api/tests.rs | Updates API integration test model setup to use metadata.cache. |
Comments suppressed due to low confidence (2)
src/registry/model_registry.rs:24
- Renaming
ModelMetadata.artifacttocachechanges the serialized JSON shape. Sincemetadatais persisted as JSON in SQLite and deserialized withserde_json::from_str, any existing rows containing{ "artifact": ... }will fail to deserialize after this change, breakingload_models()/get_model()for existing users. Consider adding backward compatibility via#[serde(alias = "artifact")]oncache(and/or a DB migration to rewrite stored JSON), and document this as a user-facing change if intentional.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct ModelMetadata {
pub cache: CacheInfo,
#[serde(skip_serializing_if = "Option::is_none")]
pub context_window: Option<u32>,
#[serde(skip_serializing_if = "Option::is_none")]
pub safetensors: Option<serde_json::Value>,
}
src/storage/sqlite.rs:256
- Tests were updated for the new
cachefield, but there’s no regression test covering backward compatibility with previously persisted JSON ({"artifact": ...}) in the SQLitemetadatacolumn. Adding a test that deserializes old metadata JSON intoModelMetadata(or loads a row containing the old key) would prevent accidental breaking changes in the future.
});
ModelInfo {
uuid: uuid.to_string(),
name: name.to_string(),
provider: "huggingface".to_string(),
author: Some("test-author".to_string()),
task: Some("text-generation".to_string()),
model_series: Some("gpt2".to_string()),
license: Some("mit".to_string()),
created_at: "2025-01-01T00:00:00Z".to_string(),
updated_at: "2025-01-01T00:00:00Z".to_string(),
metadata: ModelMetadata {
cache: CacheInfo {
revision: "abc123".to_string(),
size: 1000,
path: "/tmp/test".to_string(),
},
context_window: Some(2048),
safetensors: Some(safetensors),
},
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let metadata_json: String = row.get(7)?; | ||
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json) | ||
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; | ||
|
|
||
| Ok(ModelInfo { | ||
| uuid: row.get(0)?, | ||
| name: row.get(1)?, | ||
| provider: row.get(5)?, | ||
| author: row.get(2)?, |
There was a problem hiding this comment.
metadata is deserialized from the SQLite JSON column (serde_json::from_str). With the artifact -> cache rename in ModelMetadata, existing databases written by previous versions will contain artifact and will fail to parse here, causing load_models() / get_model() to error. Address via serde aliasing on the struct field and/or a migration that updates the stored JSON key.
Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
|
/lgtm |
Signed-off-by: kerthcet <kerthcet@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/registry/model_registry.rs:24
ModelMetadatais persisted as JSON in SQLite (seeserde_json::to_string(&model.metadata)/from_strinSqliteStorage). Renaming the JSON field fromartifacttocachewill make existing registries fail to deserialize on startup. Add a serde alias (e.g.,#[serde(alias = "artifact")]on thecachefield) or implement a custom deserializer to accept both keys for backward compatibility.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct ModelMetadata {
pub cache: CacheInfo,
#[serde(skip_serializing_if = "Option::is_none")]
pub context_window: Option<u32>,
#[serde(skip_serializing_if = "Option::is_none")]
pub safetensors: Option<serde_json::Value>,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/lgtm |
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?