Fix fingerprint when moving cache dir #2509
Conversation
|
Windows, why are you doing this to me ? |
|
Thanks @lhoestq, I'm starting reviewing this PR. |
| hasher = Hasher() | ||
| hasher.update(self._relative_data_dir().replace(os.sep, "/")) | ||
| hasher.update(split.to_spec() if isinstance(split, ReadInstruction) else str(split)) | ||
| fingerprint = hasher.hexdigest() |
There was a problem hiding this comment.
This might be extracted into a funcion/method.
src/datasets/utils/filelock.py
Outdated
| # ------------------------------------------------ | ||
|
|
||
|
|
||
| MAX_LOCK_FILENAME_LENGTH = 255 |
There was a problem hiding this comment.
Is this a parameter that the user should be allowed to customize? If yes, I would suggest moving it to config; if not (I think this is the case), then I would suggest to remove the variable and pass the value directly in the function signature of hash_filename_if_too_long.
src/datasets/utils/filelock.py
Outdated
| MAX_LOCK_FILENAME_LENGTH = 255 | ||
|
|
||
|
|
||
| def hash_filename_if_too_long(path: str, max_length=MAX_LOCK_FILENAME_LENGTH) -> str: |
There was a problem hiding this comment.
As this function is only used by FileLock, maybe it could become a method of that class: it is the class responsibility to check whether the filename is too long and hash it.
There was a problem hiding this comment.
And I would recommend adding some unit tests for this function (even if they seem quite trivial).
mariosasko
left a comment
mariosasko
left a comment
There was a problem hiding this comment.
I was hoping hash_filename_if_too_long would fix issues with FileLock on Windows if long paths are not enabled (#2443), but sadly it doesn't.
|
Yea issues on windows are about long paths, not long filenames. |
Co-authored-by: Mario Šaško <[email protected]>
|
Took your suggestions into account @albertvillanova :) |
albertvillanova
left a comment
albertvillanova
left a comment
There was a problem hiding this comment.
Once the code quality check is fixed, it is OK for me.
Thanks!
The fingerprint of a dataset changes if the cache directory is moved.
I fixed that by setting the fingerprint to be the hash of:
Close #2496
I had to fix an issue with the filelock filename that was too long (>255). It prevented the tests to run on my machine. I just added
hash_filename_if_too_longin case this happens, to not get filenames longer than 255.We usually have long filenames for filelocks because they are named after the path that is being locked. In case the path is a cache directory that has long directory names, then the filelock filename could en up being very long.