updated user permissions based on umask
Author: bhavitvyamalik
Pull request: yes

# What does this PR do?

Fixes [#2065](https://github.com/huggingface/datasets/issues/2065) where cached model's permissions change depending on running user's umask.  

## Who can review?
@thomwolf @stas00 please let me know if any other changes are required in this.


Comments:
Thank you, @bhavitvyamalik. This is excellent

Let's first review what we are trying to correct.

Looking under `~/.cache/huggingface/transformers/` I see:
```
-rw------- 1 stas stas  1.1K Oct 16 13:27 00209bab0f0b1af5ef50d4d8a2f8fb0589ec747d29d975f496d377312fc50ea7.688a102406298bdd2190bac9e0c6da7c3ac2bfa26aa40e9e07904fa
e563aeec3
-rw-rw-r-- 1 stas stas   158 Oct 16 13:27 00209bab0f0b1af5ef50d4d8a2f8fb0589ec747d29d975f496d377312fc50ea7.688a102406298bdd2190bac9e0c6da7c3ac2bfa26aa40e9e07904fa
e563aeec3.json
-rwxrwxr-x 1 stas stas     0 Oct 16 13:27 00209bab0f0b1af5ef50d4d8a2f8fb0589ec747d29d975f496d377312fc50ea7.688a102406298bdd2190bac9e0c6da7c3ac2bfa26aa40
e9e07904fae563aeec3.lock*
-rw------- 1 stas stas  4.9M Oct 14 11:56 002911b8e4cea0a107864f5b17f20c10f613d256e92e3c1247d6d174fbf56fe5.bf6ebaf6162cfbfbad2ce1909278a9ea1fbfe9284d318bff8bccddf
daa104205
-rw-rw-r-- 1 stas stas   130 Oct 14 11:56 002911b8e4cea0a107864f5b17f20c10f613d256e92e3c1247d6d174fbf56fe5.bf6ebaf6162cfbfbad2ce1909278a9ea1fbfe9284d318bff8bccddf
daa104205.json
```

So some files already have the correct perms `-rw-rw-r--`, but the others don't (`-rw-------` missing group/other perms)

If I try to get a new cached file:

```
PYTHONPATH="src" python -c "from transformers import AutoModel; AutoModel.from_pretrained('sshleifer/student_pegasus_xsum_16_8')"
```

we can see how the tempfile uses user-only perms while downloading it:
```
-rw------- 1 stas stas  246M Apr  7 10:06 tmplse9bwr1
```

and then your fix, adjusts the perms:

```
-rw-rw-r-- 1 stas stas  1.7G Apr  7 10:08 6636af980d08a3205d570f287ec5867d09d09c71d8d192861bf72e639a8c42fc.c7a07b57c0fbcb714c5b77aa08bea4f26ee23043f3c28e7c1af1153
a4bdfeea5
-rw-rw-r-- 1 stas stas   180 Apr  7 10:08 6636af980d08a3205d570f287ec5867d09d09c71d8d192861bf72e639a8c42fc.c7a07b57c0fbcb714c5b77aa08bea4f26ee23043f3c28e7c1af1153
a4bdfeea5.json
-rwxrwxr-x 1 stas stas     0 Apr  7 10:06 6636af980d08a3205d570f287ec5867d09d09c71d8d192861bf72e639a8c42fc.c7a07b57c0fbcb714c5b77aa08bea4f26ee23043f3c28
e7c1af1153a4bdfeea5.lock*
```

So this is goodness.

There is also a recipe subclassing `NamedTemporaryFile` https://stackoverflow.com/a/44130605/9201239 so it's even more atomic. But I'm not sure how that would work with resumes. I think your way is just fine for now and if we start doing more of that we will use a subclass that fixes perms internally.


That makes sense, moving it from `cached_path` to `get_from_cache`. Let me push your suggested changes. Yeah, even I came across this subclassing `NamedTemporaryFile` when I had to fix this for Datasets but I felt adding more such tempfiles and then using subclassing would be more beneficial.

Any plans for asking user what file permission they want for this model?

> Any plans for asking user what file permission they want for this model?

Could you elaborate why would a user need to do that?

For shared environment this is a domain of `umask` and may be "sticky bit".


When we started working on this feature for Datasets someone suggested this to us:

> For example, I might start a training run, downloading a dataset. Then, a couple of days later, a collaborator using the same repository might want to use the same dataset on the same shared filesystem, but won't be able to under the default permissions.
> 
> Being able to specify directly in the top-level load_dataset() call seems important, but an equally valid option would be to just inherit from the running user's umask (this should probably be the default anyway).
> 
> So basically, argument that takes a custom set of permissions, and by default, use the running user's umask!

Say if someone doesn't want the default running user's umask then they can specify what file permissions they want for that model. Incase they opt for this, we can avoid the umask part and directly `chmod` those permissions for the newly downloaded model. I'm not sure how useful would this be from the context from Transformers library.

Thank you for sharing the use case, that was helpful.

But this can be solved on the unix side of things. If you want a shared directory you can set it up as such. If you need to share files with other members of the group you put them into the same unix group.

IMHO, in general programs shouldn't mess with permissions directly. Other than the fix you just did which compensates for the temp facility restrictions.


@LysandreJik, could you please have a look so that we could merge this? Thank you!

@stas00 @bhavitvyamalik I must say that I am not familiar with the umask command, but it seems = as @LysandreJik rightfully points out in my feature request https://github.com/huggingface/transformers/issues/12169#issuecomment-861467551 - that this may solve the issue that we were having.

In brief (but please read the whole issue if you have the time): we are trying to use a single shared cache directory for all our users to prevent duplicate models. This did not work as we were running into permission errors (due to `-rw-------` as @stas00 shows). Does this PR change the behaviour of created/downloaded files so that they adhere to the permission level of the current directory? Or at least that those files are accessible by all users?

Thanks!

I think, yes, this was the point of this PR. The problem is that `tempfile` forces user-only perms, so this PR restored them back to `umask`'s setting.

One other thing that helps is setting set group id bit `g+s`, which makes sub-dirs and files create under such dirs inherit the perms of the parent dir.

So your setup can be something like:

```
sudo find /shared/path -type d -execdir chmod g+rwxs {} \;
sudo find /shared/path -type f -execdir chmod g+rw {} \;
sudo chgrp -R shared_group_name /shared/path
```

where `/shared/path` is obvious and `shared_group_name` is the group name that all users that should have access belong to.

Finally, each user having `umask  0002` or `umask 0007` in their `~/.bashrc` will make sure that the files will be read/write-able by the group on creation. `0007` is if you don't want files to be readable by others.

Note that some unix programs don't respect set gid, e.g. `scp` ignores any sub-folders copied with `scp -r` and will set them to user's `umask` perms and drop setgid. But I don't think you'll be affected by it.

Thanks, this looks promising! We currently have a "hack" implemented that simply watches for new file changes and on the creation of a new file, changes the permissions. Not ideal, but seeing that some colleagues use older versions of transformers in their experiments, we will have to make do for now.