Conversation
Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
|
/lgtm |
There was a problem hiding this comment.
Pull request overview
This PR extends the puma ls command to support optional filtering (pattern + column-based filters), and refactors CLI command implementations into dedicated modules while updating the storage layer to accept filter parameters.
Changes:
- Add
puma lsarguments for a regex pattern and-l/--querykey-value filters, wiring them through CLI → registry → storage. - Update SQLite storage + registry APIs to support filtered
load_models(...), and rename the model “type” field/column totask. - Add new CLI modules (
ls,inspect,rm) and introduce a new integration test suite.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration_test.rs | Adds end-to-end CLI tests for ls filtering and other commands (currently includes a network-dependent pull). |
| src/system/system_info.rs | Updates system info to use the new load_models(None) signature. |
| src/storage/storage_trait.rs | Extends storage trait load_models to accept optional filters. |
| src/storage/sqlite.rs | Implements SQL filtering, renames type→task in schema/queries, and normalizes stored names/authors to lowercase. |
| src/registry/model_registry.rs | Propagates new load_models(filters) API and renames r#type→task. |
| src/lib.rs | Adds minimal crate-level commentary (no functional changes). |
| src/downloader/huggingface.rs | Updates downloaded model info to populate task. |
| src/cli/rm.rs | Extracts rm logic into a module with unit tests. |
| src/cli/mod.rs | Exposes new CLI submodules. |
| src/cli/ls.rs | Implements ls filtering logic (regex + query parsing) with unit tests. |
| src/cli/inspect.rs | Extracts inspect logic + display formatting with unit tests. |
| src/cli/commands.rs | Adds LsArgs, wires new ls/rm/inspect modules, and updates ls table columns. |
| Cargo.toml | Adds regex dependency. |
| Cargo.lock | Locks regex dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "CREATE TABLE models ( | ||
| uuid TEXT PRIMARY KEY, | ||
| name TEXT NOT NULL UNIQUE, | ||
| author TEXT, | ||
| type TEXT, | ||
| task TEXT, |
There was a problem hiding this comment.
The initial migration now creates task instead of the previous type column. Existing DBs that already ran the old migration won't be updated by rusqlite_migration, so later queries selecting task will fail. Add a new migration that renames/adds the column (and index) while preserving existing data.
| // Normalize name and author to lowercase | ||
| let name_lower = model.name.to_lowercase(); | ||
| let author_lower = model.author.as_ref().map(|a| a.to_lowercase()); | ||
|
|
There was a problem hiding this comment.
Lowercasing model.name on write is a backward-incompatible change for existing registries that may have stored mixed-case names previously; combined with lowercasing lookups, those rows become unreachable. If this behavior is desired, add a DB migration to normalize existing rows; otherwise, prefer case-insensitive comparisons (e.g., COLLATE NOCASE) without rewriting stored values.
| let pattern_lower = pattern_str.to_lowercase(); | ||
| match regex::Regex::new(&pattern_lower) { |
There was a problem hiding this comment.
Lowercasing the user-provided regex pattern changes the regex semantics (e.g., character classes like [A-Z], escapes, and Unicode properties), not just case-sensitivity. Prefer compiling the original pattern with case-insensitive mode (e.g., RegexBuilder::new(pattern_str).case_insensitive(true)) or lowercasing both sides only when doing plain substring matching.
| let pattern_lower = pattern_str.to_lowercase(); | |
| match regex::Regex::new(&pattern_lower) { | |
| match regex::RegexBuilder::new(pattern_str) | |
| .case_insensitive(true) | |
| .build() | |
| { |
|
|
||
| #[derive(Parser)] | ||
| struct LsArgs { | ||
| /// Optional model name pattern to filter (e.g., qwen, openai/*) |
There was a problem hiding this comment.
The ls help text suggests patterns like openai/*, but ls treats this as a regex and openai/* is not a valid regex (it will error because * repeats the preceding token /). Either update the examples to valid regex (e.g., ^openai/.*) or implement glob-style matching if that's the intended UX.
| /// Optional model name pattern to filter (e.g., qwen, openai/*) | |
| /// Optional model name regex to filter (e.g., qwen, ^openai/.*) |
| // Pull a real model | ||
| let output = run_puma( | ||
| home, | ||
| &["pull", "inftyai/tiny-random-gpt2", "-p", "huggingface"], | ||
| ); |
There was a problem hiding this comment.
This test performs a real pull from Hugging Face over the network, which makes CI runs flaky and environment-dependent (network/rate limits/outages). Consider marking it #[ignore] by default, gating behind an env var/feature, or using a mocked/local provider so tests run offline deterministically.
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?