Set correct permissions on blob file

[Wauplin]

Fix #1141 #1215.

Workaround I used is to infer umask from the permission set on the blobs/ folder. This is not optimal (not getting the umask from the user directly) but good enough as at least all folders/files in cache folder will have same permissions.

    # src is a tmp file, dst is the blob path
    cache_dir_mode = Path(dst).parent.stat().st_mode
    os.chmod(src, stat.S_IMODE(cache_dir_mode))
    os.replace(src, dst)

The reason behind it that getting the umask from the user is risky. The python implementation consists in changing the value while getting the old value. Can lead to weird/undesired issues in a multi-thread process. I found a potentially thread-safe solution but didn't want to have to maintain/debug it on multiple platform (solution is more a hack than a proper solution).

cc @stas00 @antoche what do you think ? Looks good to you ? (Also cc @julien-c who worked on the new cache)

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[stas00]

in the proposed fix dirmode != filemode - different bits

I wonder if the simplest way here is to cheat and copy the same perms as some other files other than blob files, since those will be already correctly set as tmpfile isn't used. for example refs/main could be the source of truth?

[Wauplin]

in the proposed fix dirmode != filemode - different bits

Aaarrg 😭 😭

I wonder if the simplest way here is to cheat and copy the same perms as some other files other than blob files, since those will be already correctly set as tmpfile isn't used. for example refs/main could be the source of truth?

We cannot even do that. If the user downloads a file from a specific commit hash, there is no other file created than the blob itself (only folders and a symlink). Especially, there is a refs/main file only when users download the latest revision. This is the case 99% of the time but for the 1% remaining, this is a real problem.

# Nothing in `refs/` !
filepath = hf_hub_download(repo_id, "config.json", revision="e9a30121a64df2604d118f7d209411c7202e09c3")

---

The last solution I see is to create a dumb file with a uuid, check the filemode and delete it. In 99,99% of cases, that would do the trick. Only problem is if the process is shut down while the dumb file still exists (as we cannot do it in a single operation). User would end up with an empty file .cache/tmp_f32db9df-3b5f-4d63-a01c-706e45d794f1 which is not really problematic right. WDYT @stas00 ?

[codecov]

Codecov Report

Base: 84.27% // Head: 84.31% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (a77aad8) compared to base (c450acd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
+ Coverage   84.27%   84.31%   +0.03%     
==========================================
  Files          44       44              
  Lines        4338     4348      +10     
==========================================
+ Hits         3656     3666      +10     
  Misses        682      682              

Impacted Files	Coverage Δ
src/huggingface_hub/file_download.py	88.43% <100.00%> (+0.34%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Wauplin]

@stas00 I made a fix in a77aad8 to check current filemode. Can you cross-check what I did is correct ? Thanks in advance :)

[stas00]

I validated your changes in action and I confirm that the problem has been fixed. Thank you, @Wauplin!

I think your approach of creating a non-/tmp temp file to get the target file perms is close to perfection :)

wrt a potential dropped file, at some point we discussed cache purging functionality, why not add a TODO list to clean up any tmp_* files in the root of the cache folder whenever that purge is called by the user.

Once merged if possible let's make a new release as this is somewhat a problem with many users each having their own conda env, so it'd be more difficult to ask everybody to work from the bleed edge. Thank you!

[Wauplin]

wrt a potential dropped file, at some point we discussed cache purging functionality, why not add a TODO list to clean up any tmp_* files in the root of the cache folder whenever that purge is called by the user.

👍

Once merged if possible let's make a new release as this is somewhat a problem with many users each having their own conda env, so it'd be more difficult to ask everybody to work from the bleed edge. Thank you!

I'm not against the idea of making a hot-fix release for that if it impacts a lot of users. But I'm not so sure it's the case as the new cache has been release a few months ago and we did not have a lot of requests in that direction.

[LysandreJik]

Sounds like a reasonable change, have you tried/would you like me to try it on windows?

[Wauplin]

have you tried/would you like me to try it on windows?

Thanks for the idea ! I gave it a try and it works as expected :) However the actual test is failing on windows. I still need to figure out how to fix that when setting up a windows CI (but can be done in a future PR)

[stas00]

I'm not against the idea of making a hot-fix release for that if it impacts a lot of users. But I'm not so sure it's the case as the new cache has been release a few months ago and we did not have a lot of requests in that direction.

FWIW, all of HF who works on JeanZay is impacted.

[Wauplin]

FWIW, all of HF who works on JeanZay is impacted.

Hmm, sorry it wasn't raised before then. I can make a hot-release v0.11.1 on Monday then (don't want to release on a Friday evening :) )

[stas00]

Any time soon is no problem, I was just hoping it won't be weeks before the next release.
I asked everybody on JZ to meanwhile run pip install git+https://github.com/huggingface/huggingface_hub until then.