-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#2837 Use cache folder for lockfile #2887
#2837 Use cache folder for lockfile #2887
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change, I just have two comments:
src/datasets/load.py
Outdated
@@ -452,8 +452,12 @@ def prepare_packaged_module(name): | |||
|
|||
if force_local_path is None: | |||
main_folder_path = os.path.join(datasets_modules_path if dataset else metrics_modules_path, short_name) | |||
# Create the lock file where we know we have write permissions. | |||
lock_path = main_folder_path + ".lock" | |||
os.makedirs(main_folder_path, exist_ok=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must notice line 678 that if force_download is set to True, the main_folder_path
directory is deleted, which is an issue because it contains an open lock file. Therefore I would put the lock file it inside datasets_modules_path if dataset else metrics_modules_path
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thank you!
Something like:
# Create the lock file where we know we have write permissions.
lock_path = (datasets_modules_path if dataset else metrics_modules_path) + f"{name}.lock"
so that each dataset has their own lock file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
The CI fail about the meteor metric is unrelated to this PR |
Fixes #2837
Use a cache folder directory to store the FileLock.
The issue was that the lock file was in a readonly folder.