Repository navigation
Conversation
| // Cross-process exclusive lock (flock on POSIX, LockFileEx on Windows). | ||
| // The constructor only stores the path: acquire() opens the lock file and | ||
| // waits out contention, so code that never acquires the lock (pure cache | ||
| // reads) works on a read-only cache. |
There was a problem hiding this comment.
need to clean up comments per AGENTS.md
There was a problem hiding this comment.
I literally asked to add some comments and to be verbose (mostly to help the LLM itself) because locking correctly is actually hard. The model often failed without being driven correctly...
But maybe it became too verbose at some points. I'll do a pass
There was a problem hiding this comment.
maybe we don't need a new cpp/h file for it, better to just move it to download.cpp as a private struct
There was a problem hiding this comment.
The file locking mechanism is generic enough and already used in two places: download and hf-cache (which is why it was moved out of download). Also it could be useful elsewhere, like modelscope, docker or when not downloading at all.
There was a problem hiding this comment.
Also it could be useful elsewhere, like modelscope, docker or when not downloading at all.
other model sources must eventually call download.h to download the actual file, so I think it should still part of the download.h
when not downloading at all.
when not downloading at all, we never need to lock anything because we should never write anything
0669e78 to
09e13f3
Compare
|
I was about to send a CI fix for the test_router_download_conflicts flake and found the fix is already here: fs_write_atomic from #29642 uses a fixed "<path>.tmp", so the router and the downloading child collide on refs/main, while the unique O_EXCL tmp in this PR's safe_write_file avoids it. |
ngxson
left a comment
ngxson
left a comment
There was a problem hiding this comment.
sorry forgot to review this, cc @ServeurpersoCom for a final check up before merging
Signed-off-by: Adrien Gallouët <angt@huggingface.co>
09e13f3 to
43d9ef4
Compare
|
Rebased |
ServeurpersoCom
left a comment
ServeurpersoCom
left a comment
There was a problem hiding this comment.
Tested the router download path and concurrent -hf processes on a shared cache, including a killed winner and the previously flaky test_router_download_conflicts (15/15): all good, LGTM!
|
@ngxson rebased and tested on my side, could you re-approve? The last push is mine so my approve does not cover it. |
|
Thanks @ServeurpersoCom! @ngxson, we can reorganize better the |
|
yes let's reorganize it later |
Signed-off-by: Adrien Gallouët <angt@huggingface.co>
Overview
This PR adds cross-process locking to the download path, so multiple processes (including the
hftool) can download the same file without corrupting it.Additional information
Supersedes #28803
Requirements