-
Notifications
You must be signed in to change notification settings - Fork 82
Get remote models #72
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
Conversation
p1-0tr
left a comment
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.
LGTM
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.
Pull Request Overview
Adds support for fetching models from remote registries via a ?remote=true flag and makes the Tags field optional in responses.
- Introduces a registry client alongside the existing distribution client.
- Splits
handleGetModelintogetLocalModelandgetRemoteModelpaths. - Updates the
ModelJSON schema to omittagswhen empty.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/inference/models/manager_test.go | Adds TestHandleGetModel covering both local and remote retrieval but has import/alias mismatches. |
| pkg/inference/models/manager.go | Registers a registryClient, parses remote query param, and delegates to getLocalModel/getRemoteModel. |
| pkg/inference/models/api.go | Updates Model.Tags JSON tag to use omitempty. |
Comments suppressed due to low confidence (4)
pkg/inference/models/manager_test.go:7
- Test uses strings.Contains and filepath.Join but misses imports for "strings" and "path/filepath"; add these imports to the import block.
net/http/httptest
pkg/inference/models/manager.go:283
- [nitpick] Remote model mapping initializes
Tagsas empty, dropping any tags present in the registry model; consider populating this slice frommodel.Tags()or equivalent source.
Tags: make([]string, 0),
pkg/inference/models/manager_test.go:152
- Imported registry package is aliased as
reg, but code callsregistry.New(). Change toreg.New()to match the import alias.
server := httptest.NewServer(registry.New())
pkg/inference/models/manager.go:204
- Missing import for the
strconvpackage required forParseBool. Addimport "strconv".
if val, err := strconv.ParseBool(r.URL.Query().Get("remote")); err != nil {
doringeman
left a comment
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.
Thanks, LGTM!
…ontent (#72) fix: use empty string instead of nullptr for templates requiring non null
Adds ability to query models from remote registries via
?remote=truequery parameter.Tagsfield optional in JSON responsesGET /models/{name}?remote=true- Fetch model from remote registry@doringeman We initially discussed creating a common type for local and remote models, but I ultimately decided against it because I don’t see a strong need.
We use different clients to retrieve local vs. remote models (the distribution client for local and the registry client for remote), and each returns its own type, which makes sense given their responsibilities.
For example, in the case of local models, it's important to return a Model in order to provide
GGUFPath()