Only lock when actually downloading a model and per model_path#790
Only lock when actually downloading a model and per model_path#790simo-prior merged 4 commits intomainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request refactors the model download locking mechanism from a single global lock to a more granular, per-file lock, which is a great improvement for concurrent downloads of different models. However, a security audit identified a medium-severity path traversal vulnerability in the download locking mechanism, which could be exploited for Denial of Service attacks. Additionally, this change will likely break the existing concurrency test test__load_model_criterion_config__parallel_downloads_do_not_crash because the @_with_download_lock decorator was moved from load_model_criterion_config to download_model, and the test currently mocks download_model directly, bypassing the new locking mechanism. The test needs to be updated to correctly verify the new locking mechanism. A suggestion has also been provided to improve the robustness of the new decorator.
noahho
left a comment
There was a problem hiding this comment.
I didn't go very deep but this looks good to me and very well engineeered
Issue
https://linear.app/priorlabs/issue/ENG-509/filenotfounderror-level-error-errno-2-no-such-file-or-directory-tabpfn
Motivation and Context
Previously we were applying a global lock on every load_model_criterion_config() call to prevent race conditions while downloading models. This call would not necessarily result in a download and different model_paths do not require a shared lock.