Support sqllite to store model information#32
Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
InftyAI-Agent
added
needs-triage
There was a problem hiding this comment.
Pull request overview
Adds a SQLite-backed storage layer for model registry data, replacing the prior JSON-file registry approach.
Changes:
- Introduces a
ModelStoragetrait and aSqliteStorageimplementation with migrations and CRUD operations. - Refactors
ModelRegistryto use SQLite storage and updates CLI/downloader code to the newModelInfo/metadata structure. - Adds
rusqlite/rusqlite_migrationdependencies and unit tests for the new storage.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/storage_trait.rs | Defines the storage backend trait used by the registry. |
| src/storage/sqlite.rs | Implements SQLite persistence, schema migrations, and tests. |
| src/storage/mod.rs | Exposes storage modules and re-exports the public API. |
| src/registry/model_registry.rs | Switches registry persistence from JSON to SQLite and updates model structures. |
| src/downloader/huggingface.rs | Populates the new ModelInfo/ModelMetadata fields when registering downloads. |
| src/cli/commands.rs | Updates ls/inspect output to read from the new metadata layout and updates tests. |
| src/main.rs | Registers the new storage module in the binary crate. |
| Cargo.toml / Cargo.lock | Adds SQLite-related dependencies and lockfile updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if model exists to preserve created_at | ||
| let existing_created_at: Option<String> = conn | ||
| .query_row( | ||
| "SELECT created_at FROM models WHERE name = ?1", | ||
| params![&model.name], | ||
| |row| row.get(0), | ||
| ) | ||
| .ok(); | ||
|
|
There was a problem hiding this comment.
query_row(...).ok() will convert any SQL error into None, not just “no rows”. That can silently mask unexpected failures (schema issues, IO/corruption, etc.) and change behavior. Prefer matching on the error and only treating QueryReturnedNoRows as None, while propagating other errors.
| pub fn new(home_dir: Option<PathBuf>) -> Self { | ||
| Self { | ||
| home_dir: home_dir.unwrap_or_else(file::root_home), | ||
| } | ||
| } | ||
| let home_dir = home_dir.unwrap_or_else(file::root_home); | ||
| fs::create_dir_all(&home_dir).ok(); | ||
|
|
||
| fn registry_file(&self) -> PathBuf { | ||
| self.home_dir.join("models.json") | ||
| } | ||
|
|
||
| pub fn load_models(&self) -> Result<Vec<ModelInfo>, std::io::Error> { | ||
| let registry_file = self.registry_file(); | ||
| let db_path = home_dir.join("models.db"); | ||
| let storage = SqliteStorage::new(db_path).expect("Failed to initialize storage"); | ||
|
|
||
| if !registry_file.exists() { | ||
| return Ok(Vec::new()); | ||
| Self { | ||
| storage: Box::new(storage), | ||
| } |
There was a problem hiding this comment.
ModelRegistry::new ignores errors from create_dir_all and panics on SQLite init (expect). This makes failures (e.g., permission issues, unwritable home dir) crash the CLI instead of being reported/handled. Consider changing new to return Result<Self, io::Error> (or adding a try_new) and propagate both directory-creation and SqliteStorage::new errors.
|
/lgtm |
InftyAI-Agent
added
the
lgtm
Signed-off-by: kerthcet <kerthcet@gmail.com>
InftyAI-Agent
removed
the
lgtm
|
/lgtm |
|
/kind feature |
InftyAI-Agent
added
lgtm
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json) | ||
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; |
There was a problem hiding this comment.
JSON deserialization errors while reading metadata are being converted into rusqlite::Error::ToSqlConversionFailure, which is a misleading error variant for a FROM-SQL conversion. Prefer rusqlite::Error::FromSqlConversionFailure (or map the serde error directly into an io::Error/custom error) so failures are classified correctly.
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json) | |
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; | |
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json).map_err( | |
| |e| { | |
| rusqlite::Error::FromSqlConversionFailure( | |
| 7, | |
| rusqlite::types::Type::Text, | |
| Box::new(e), | |
| ) | |
| }, | |
| )?; |
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json) | ||
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; |
There was a problem hiding this comment.
Same as load_models: JSON deserialization errors for the metadata column are mapped to rusqlite::Error::ToSqlConversionFailure, which is misleading for a read/FromSql path. Use FromSqlConversionFailure (or map to an io::Error) so error reporting is accurate.
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json) | |
| .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; | |
| let metadata: ModelMetadata = serde_json::from_str(&metadata_json).map_err( | |
| |e| { | |
| rusqlite::Error::FromSqlConversionFailure( | |
| 7, | |
| rusqlite::types::Type::Text, | |
| Box::new(e), | |
| ) | |
| }, | |
| )?; |
| updated_at: "2025-01-01T00:00:00Z".to_string(), | ||
| metadata: ModelMetadata { | ||
| artifact: ArtifactInfo { | ||
| revision: "abc123".to_string(), |
There was a problem hiding this comment.
In the SQLite tests, create_test_model takes a uuid parameter but hard-codes metadata.artifact.revision to "abc123". This makes the fixture internally inconsistent and can mask bugs where UUID/revision are expected to align; consider using the passed-in value for the artifact revision too (or rename the parameter to clarify intent).
| revision: "abc123".to_string(), | |
| revision: uuid.to_string(), |
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?