[Feature] Model Agent support diffusers models.#520
[Feature] Model Agent support diffusers models.#520shenoyvvarun wants to merge 2 commits intosgl-project:mainfrom
Conversation
shenoyvvarun
requested review from
CatherineSue,
XinyueZhang369,
beiguo218,
pallasathena92,
slin1237 and
truddy0
as code owners
github-actions
bot
added
models
Summary of ChangesHello @shenoyvvarun, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the model agent's capabilities by introducing native support for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for diffusers models in the model agent by parsing model_index.json. The changes include prioritizing model_index.json over config.json, updating the model spec with diffusion pipeline metadata, and adding corresponding parsing logic and tests. A bug fix for Hugging Face repository URL construction is also included. The changes are well-implemented and tested. I have two suggestions: one to address a potential bug by ensuring consistency in URL escaping for different repository types, and another to refactor duplicated code for better maintainability.
| switch repoType { | ||
| case RepoTypeModel: | ||
| apiURL = fmt.Sprintf("%s/api/models/%s/tree/%s?recursive=true", endpoint, url.PathEscape(config.RepoID), url.QueryEscape(revision)) | ||
| apiURL = fmt.Sprintf("%s/api/models/%s/tree/%s?recursive=true", endpoint, config.RepoID, url.QueryEscape(revision)) |
There was a problem hiding this comment.
While removing url.PathEscape for RepoTypeModel is correct, this introduces an inconsistency. RepoTypeDataset and RepoTypeSpace still use url.PathEscape, but their repository IDs can also contain slashes (e.g., mozilla-foundation/common_voice_11_0) which would lead to the same 404 error. It is recommended to apply this fix consistently across all repository types that can have slashes in their IDs to prevent future bugs.
There was a problem hiding this comment.
The comment is fair, this is dataset and space too. I can make similar changes for others too based on reviwer feedback.
| // findModelIndexFile searches for the model_index.json file in the model directory | ||
| // It checks the root directory and common subdirectories | ||
| func (p *ModelConfigParser) findModelIndexFile(modelDir string) (string, error) { | ||
| // Check the root directory first | ||
| rootIndexPath := filepath.Join(modelDir, DefaultModelIndexFileName) | ||
| if _, err := os.Stat(rootIndexPath); err == nil { | ||
| return rootIndexPath, nil | ||
| } | ||
|
|
||
| // If not found in rootdir, do a recursive search (limited to avoid deep searching) | ||
| var indexPath string | ||
| err := filepath.Walk(modelDir, func(path string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return nil // Skip error files | ||
| } | ||
| if !info.IsDir() && info.Name() == DefaultModelIndexFileName { | ||
| indexPath = path | ||
| return filepath.SkipDir // Found it, stop searching | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if indexPath == "" { | ||
| return "", fmt.Errorf("model_index.json not found in %s", modelDir) | ||
| } | ||
| return indexPath, nil | ||
| } |
There was a problem hiding this comment.
The new findModelIndexFile function has very similar logic to the existing findConfigFile function. To improve maintainability and avoid code duplication, consider refactoring these two functions into a single, generic findFile helper function. This new function could take the filename and a list of common subdirectories to check as arguments.
There was a problem hiding this comment.
This is similar but, there is slight difference. The only logic that is shared is the sub-directory lookup. I feel it makes the code look clean but, more than willing to change the code based on feedback.
github-actions
bot
added
auth
shenoyvvarun
force-pushed
the
vasheno/diffUsersModelAgentSupport
branch
from
ae20c4e to
5b7ec88
Compare
1 participant
What this PR does
PR #489 added API support for diffusion models API and also matcher support to automatically pick the right runtime. This adds the support for custom model import by adding support in OME model-agent.
Changes
Revision 2
Minor bug fix
Why we need it
How to test
config/models/Qwen/Qwen-Image.yamlChecklist
make testpasses locally