Repository navigation
Conversation
|
Hi @yrk111222, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
513104d to
1e4d68b
Compare
|
HI @angt: |
|
@yrk111222 That looks much better indeed, well done! |
|
Hi @angt, I noticed the recent large refactor to the download code, and this PR has fallen a bit behind. To minimize rework, would you rather I rebase it against the current state now, or hold off until things stabilize? Happy to follow your lead on this—just let me know what works best. Thanks!😊 |
Yes we are sorry for that, and I think we still need some refactoring. |
1e4d68b to
4184079
Compare
|
Hi @angt, Rebased onto the latest master and synced the MS code with the download refactor. Build + tests pass. Thanks!😊 |
c370cb6 to
f456bc5
Compare
|
Hi @angt, rebased on latest master and aligned MS with the HF download pipeline changes. Key updates since last review:
Would appreciate another review when you have a moment.😊 |
01f1ea7 to
0a1767a
Compare
|
@angt could you please take another look and (hopefully) get this merged soon? this has been outstanding for a (unfortunate) long period of time... thanks! |
|
a gentle ping again @angt hope we can get this merged before we need to resolve conflict again ... :( |
1d9c60e to
c1efb35
Compare
|
Hi @angt, gentle ping again. Rebased on latest master and resolved conflicts. Is the refactoring stabilized enough for another PR review? Happy to address any feedback promptly... : ) |
|
this change has been pending for over two months (if we count the original implementation #22279 , then it is over 5 months), with multiple merges to keep it up to date. is there any outstanding issue that is blocking the merge? we would really love to see things moving forward if the code is now in good shape :) thanks for your time. @angt |
c1efb35 to
2dc6eca
Compare
|
Hi @angt, |
Overview
This PR adds
ModelScope (-ms)option support. Based on the feedback #22279 about keeping responsibilities separated and following thehf-cache.cpppatternAdditional information
Key Design Decisions
1. Downloading, quantization selection, and filtering logic stays in download.cpp
Following the reviewer's request, we did not duplicate these functions in ms-cache.cpp. Instead, the existing functions in download.cpp were templatized so they can be shared by both HF and MS. This keeps ms-cache.cpp focused on caching only — mirroring hf-cache.cpp's responsibilities. The templates are defined entirely within download.cpp, with no header changes, so HF behavior is unaffected.
2. ms-cache.cpp follows the pattern of hf-cache.cpp closely
ms_file struct mirrors hf_file (identical fields)
Public API matches hf-cache.h (get_repo_files, get_cached_files, finalize_file)
Same blob/snapshot/symlink cache structure
The only differences are platform-specific (unavoidable):
Auth: Cookie vs Bearer
Endpoint: dedicated ModelScope endpoint
Snapshot key: "master" (MS API has no single repo-wide commit hash)
oid: Sha256 from API
3. Cache path aligned with HF + token-less loading
MS uses refs/master as the snapshot key. Since MS files use regular filenames requiring ETag validation (unlike HF's content-addressed blobs), cached MS models need extra handling to load without a token — just like HF. When all files are cached, download tasks are skipped and cached paths are returned directly.
Usage
Requirements
I used an LLM to translate the PR description and do some code review.