Repository navigation
fix(model-manager): filter repo-id Diffusers files by index - #9912
lorenzozanee wants to merge 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
python
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db09264c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| submodels = resp.json() | ||
| paths = [Path(subfolder or "", x) for x in paths if Path(x).parent.as_posix() in submodels] | ||
| paths.insert(0, Path(f"{prefix}model_index.json")) | ||
| paths = [x for x in paths if x.relative_to(model_root).parent.as_posix() in submodels] |
There was a problem hiding this comment.
When installing MiniMaxAI/MiniMax-H3 by repo ID, filter_files() selects both root indexes (tests/backend/model_manager/util/test_hf_model_select.py:419-420), but modular_model_index.json has relative parent . and is removed by this predicate before only the classic index is reinserted. Main_Diffusers_MiniMaxH3_Config.from_model_on_disk() requires the modular index to identify H3 (invokeai/backend/model_manager/configs/main.py:1757-1763), so a normal full-repo install is left unrecognizable; retain this required root pipeline index while filtering unrelated root artifacts.
AGENTS.md reference: invokeai/backend/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if Path(f"{prefix}model_index.json") in paths: | ||
| model_root = repo_root / prefix | ||
| model_index_path = model_root / "model_index.json" | ||
| if model_index_path in paths: |
There was a problem hiding this comment.
When a repository exposes an actual variant revision (for example fp16) whose index differs from its default branch, this newly reachable condition filters the variant's files with the wrong index: HuggingFaceMetadataFetch.from_id() builds each file URL with revision=variant (metadata/fetch/huggingface.py:123-130), while the request below reconstructs model_index.json without a revision. Required variant-only components can then be silently omitted; request the URL already stored for model_index_path, which also records whether metadata fetching fell back to the default revision.
AGENTS.md reference: invokeai/backend/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
Now that the index check is reachable, gated and private repos go through it too. The The same call runs during Something like this keeps both cases working: if resp.status_code in (401, 403):
# gated or private repo: keep the unfiltered selection and let the file downloads report access
return [x for x in self.files if x.path in paths]
if resp.status_code == 429 or resp.status_code >= 500:
raise ModelMetadataUnavailableError(f"Could not fetch model index for '{self.id}': HTTP {resp.status_code}")
resp.raise_for_status() |
| # to the model (only for single subfolder case) | ||
| if Path(f"{prefix}model_index.json") in paths: | ||
| model_root = repo_root / prefix | ||
| model_index_path = model_root / "model_index.json" |
There was a problem hiding this comment.
[P1] Authenticate the newly enabled model-index request
Recognizing repository-prefixed paths now makes normal repo-id installs execute the index GET below, which uses a plain requests.Session without Hugging Face credentials. HuggingFaceMetadataFetch.from_id() can successfully retrieve a private repository through the authenticated HfApi, and the installer supplies the saved token to the download queue, but neither credential path reaches this request. Consequently, an otherwise authorized private or gated Diffusers install now fails with HTTP 401/403 before downloads start; the base revision skipped this request for these paths. Fetch the index with the same authentication context as the repository/download operations.
Pfannkuchensack
left a comment
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Requesting changes. The index filter itself is right, but now that this branch runs for real installs, the gated starter models stop installing.
I reviewed against current main (v7). The PR merges cleanly there. Nothing upstream has touched metadata_base.py or select_hf_files.py since your base, so #9901 is still live on main.
Verified: I compared the old and new file selection for the 30 plain repo-id starter repos that have a root model_index.json. This used metadata only, with the default and fp16 variants.
- 27 repos are unchanged.
- The SDXL refiner drops the unlisted
vae_1_0/, and SD3.5 L/M drop their root example-workflow JSONs. Both drops are intended. - No standard pipeline loses a component config, tokenizer, scheduler file or shard index.
With the PR merged into main, your tests, test_hf_model_select.py, test_huggingface_metadata_fetch_timeout.py and tests/app/services/model_install/ pass, and ruff is clean.
Before merge
- The points raised earlier are still open. The head only has main merges since 7db0926.
- The
model_index.jsonGET is anonymous. The production installer doesn't inject a session, sodownload_urls()falls back to a plainSession().- The anonymous request gets a 401 for eight starter repos:
stabilityai/stable-diffusion-3.5-large,stabilityai/stable-diffusion-3.5-medium,black-forest-labs/FLUX.2-dev,black-forest-labs/FLUX.2-klein-9B,ideogram-ai/ideogram-4-fp8,ideogram-ai/ideogram-4-nf4,krea/Krea-2-Rawandkrea/Krea-2-Turbo. - The install then raises
HTTPErrorbefore any file is queued, also for users who are logged in to HF. - On restart the same error errors out a restored job and removes its partial download.
test_model_index_nontransient_http_error_is_not_reclassifiedcurrently pins that raise, so it needs to change with the fix.
- The anonymous request gets a 401 for eight starter repos:
- A full
MiniMaxAI/MiniMax-H3install losesmodular_model_index.json, so the model no longer identifies. I confirmed this on the live file list. Skipping itsFL2VA/andRef2VA/sub-pipelines is a nice side effect, though. - The index comes from
maineven when the files were listed from a variant revision.
- The
Should fix
- Nested subfolders on Windows build a broken index URL (inline).
- Tests: please cover the gated (401/403) path, the kept modular index and the variant revision. The older
download_urlstests intest_huggingface_metadata_fetch_timeout.pystill use unprefixed paths. Moving them toPath(<repo-name>, ...)like your new tests would letrepo_rootdrop its fallback (inline).
| model_root = repo_root / prefix | ||
| model_index_path = model_root / "model_index.json" | ||
| if model_index_path in paths: | ||
| url = hf_hub_url(self.id, filename="model_index.json", subfolder=str(subfolder) if subfolder else None) |
There was a problem hiding this comment.
On Windows, str(subfolder) uses backslashes for a nested subfolder. For example, hf_hub_url(..., subfolder=str(Path("FL2VA/sub"))) gives .../resolve/main/FL2VA%5Csub/model_index.json. That 404s, and the 404 now raises out of the install.
The line predates this PR, but this PR is what makes it reachable. subfolder.as_posix() fixes it.
|
|
||
| paths = filter_files([x.path for x in self.files], variant, subfolder, subfolders) # all files in the model | ||
|
|
||
| repo_root = Path(self.name) if paths and paths[0].parts[0] == self.name else Path() |
There was a problem hiding this comment.
nit: In production every path starts with the repo name, because from_id() builds Path(name, rfilename). So the Path() fallback only serves the older unprefixed tests.
If the two ever disagree, the filter silently switches itself off again, which is exactly #9901. I'd use Path(self.name) unconditionally and move those tests to prefixed paths.
This branch has not been deployed
4 participants
Summary
Repo-id Diffusers installs currently skip classic
model_index.jsonfiltering because metadata paths include the repository root. This can include files outside the pipeline component folders. Normalize index lookup and component membership relative to the repository root and selected model subfolder, while preserving the original paths.Related Issues / Discussions
Fixes #9901
QA Instructions
uv run --no-sync pytest -q tests/backend/model_manager/test_huggingface_metadata_download_urls.py tests/backend/model_manager/test_huggingface_metadata_fetch_timeout.py tests/backend/model_manager/util/test_hf_model_select.py: 22 passed.git diff --checkpassed.Review
No material review findings remain. Regression coverage includes repository-root and selected-subfolder paths. Hosted CI and other remote repository layouts have not been verified.
Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.