Only user permission of saved cache files, not group
State: closed | Author: lorr1 (NONE)
Labels: name: enhancement

name: good first issue

Hello,

It seems when a cached file is saved from calling `dataset.map` for preprocessing, it gets the user permissions and none of the user's group permissions. As we share data files across members of our team, this is causing a bit of an issue as we have to continually reset the permission of the files. Do you know any ways around this or a way to correctly set the permissions?

Comments: Hi ! Thanks for reporting.

Currently there's no way to specify this.

When loading/processing a dataset, the arrow file is written using a temporary file. Then once writing is finished, it's moved to the cache directory (using `shutil.move` [here](https://github.com/huggingface/datasets/blob/f6b8251eb975f66a568356d2a40d86442c03beb9/src/datasets/arrow_dataset.py#L1646))

That means it keeps the permissions specified by the `tempfile.NamedTemporaryFile` object, i.e. `-rw-------` instead of `-rw-r--r--`. Improving this could be a nice first contribution to the library :)

Hi @lhoestq,
I looked into this and yes you're right. The `NamedTemporaryFile` is always created with mode 0600, which prevents group from reading the file. Should we change the permissions of `tmp_file.name` [here](https://github.com/huggingface/datasets/blob/f6b8251eb975f66a568356d2a40d86442c03beb9/src/datasets/arrow_dataset.py#L1871) and [here](https://github.com/huggingface/datasets/blob/f6b8251eb975f66a568356d2a40d86442c03beb9/src/datasets/arrow_dataset.py#L1590), post creation to 0644 inorder for group and others to read it?

Good idea :) we could even update the permissions after the file has been moved by shutil.move [here](https://github.com/huggingface/datasets/blob/f6b8251eb975f66a568356d2a40d86442c03beb9/src/datasets/arrow_dataset.py#L1899) and [here](https://github.com/huggingface/datasets/blob/f6b8251eb975f66a568356d2a40d86442c03beb9/src/datasets/arrow_dataset.py#L1646) actually.
Apparently they set the default 0600 for temporary files for security reasons, so let's update the umask only after the file has been moved

Would it be possible to actually set the umask based on a user provided argument? For example, a popular usecase my team has is using a shared file-system for processing datasets. This may involve writing/deleting other files, or changing filenames, which a -rw-r--r-- wouldn't fix. 

Note that you can get the cache files of a dataset with the `cache_files` attributes.
Then you can `chmod` those files and all the other cache files in the same directory.

Moreover we can probably keep the same permissions after each transform. This way you just need to set the permissions once after doing `load_dataset` for example, and then all the new transformed cached files will have the same permissions.
What do you think ?

This means we'll check the permissions of other `cache_files` already created for a dataset before setting permissions for new  `cache_files`?

You can just check the permission of `dataset.cache_files[0]` imo

> This way you just need to set the permissions once after doing load_dataset for example, and then all the new transformed cached files will have the same permissions.

I was referring to this. Ensuring that newly generated `cache_files` have the same permissions

Yes exactly

I imagine users can first do `load_dataset`, then chmod on the arrow files. After that all the new cache files could have the same permissions as the first arrow files. Opinions on this ?

Sounds nice but I feel this is a sub-part of the approach mentioned by @siddk. Instead of letting the user set new permissions by itself first and then making sure newly generated files have same permissions why don't we ask the user initially only what they want? What are your thoughts?

Yes sounds good. Should this be a parameter in `load_dataset` ? Or an env variable ? Or use the value of `os.umask` ?

Ideally it should be a parameter in `load_dataset` but I'm not sure how important it is for the users (considering only important things should go into `load_dataset` parameters)

I think it's fairly important; for context, our team uses a shared file-system where many folks run experiments based on datasets that are cached by other users.

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!

Maybe let's start by defaulting to the user's umask !
Do you want to give it a try @bhavitvyamalik ?

Yeah sure! Instead of using default `0o644` should I first extract umask of current user and then use `os.umask` on it? We can do it inside `Dataset` class so that all folders/files created during the call use running user's umask



You can get the umask using `os.umask` and then I guess you can just use `os.chmod` as in your previous PR, but with the right permissions depending on the umask.

FWIW, we have this issue with other caches - e.g. `transformers` model files. So probably will need to backport this into `transformers` as well.

thanks @thomwolf for the pointer.

Hi @stas00,
For this should we use the same umask code in the respective model directory inside `TRANSFORMERS_CACHE`?

That sounds very right to me, @bhavitvyamalik 

The cluster I am working on does not allow me to change the permission of the files with os.chmod. I was wondering if there is any workaround for this? My cache is in a GCP bucket and I can't change file permissions once I mount it.

@vmurahari3 what error do you have exactly ?

I get a permission denied error on https://github.com/huggingface/datasets/blob/b8363e0539c6f0cb5de49af32962cf2eb4c47395/src/datasets/arrow_dataset.py#L2799. I suspect I don't have permissions to change group permissions. I am mounting a GCP bucket through [gcsfuse](https://github.com/GoogleCloudPlatform/gcsfuse). 

What @lhoestq is asking for is the full multi-line traceback  - it's almost never enough to show the last line - a full stack is needed to get the context. Thank you!

I wonder if a workaround is to try/except and then issue a warning if this fails?

Hello, I'm working on a project with a very similar setup to the one mentioned by @siddk, namely we have a shared cache directory in the team that we wish to use to avoid redundant dataset downloads. However, we're hitting `PermissionError` when one member of the team tries to reload a dataset that was downloaded by another team member (stack trace below).

Concretely, we first create a shared directory and give everyone read, write, execute permissions as follows (I know this isn't best practice 🤫):

```shell
mkdir shared-folder
chmod -R 777 shared-folder
```

We then set the following in our `.bashrc` profiles:

```
# Hugging Face caches
export HUGGINGFACE_HUB_CACHE=/path/to/shared-folder
export HF_DATASETS_CACHE=/path/to/shared-folder

# For shared access to the shared-folder directory
umask 000
```

Now, running e.g. `load_dataset("emotion")` the first time works (as expected), but when another team member tries to load from cache we get something like:

```
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 1759, in load_dataset
    builder_instance = load_dataset_builder(
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 1496, in load_dataset_builder
    dataset_module = dataset_module_factory(
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 1218, in dataset_module_factory
    raise e1 from None
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 1193, in dataset_module_factory
    ).get_module()
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 903, in get_module
    local_path = self.download_loading_script()
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/load.py", line 871, in download_loading_script
    return cached_path(file_path, download_config=download_config)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/utils/file_utils.py", line 210, in cached_path
    output_path = ExtractManager(cache_dir=download_config.cache_dir).extract(
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/utils/extract.py", line 42, in extract
    extractor_format = self.extractor.infer_extractor_format(input_path)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/utils/extract.py", line 287, in infer_extractor_format
    if extractor.is_extractable(path, magic_number=magic_number):
  File "~/miniconda3/envs/llama-nol/lib/python3.10/site-packages/datasets/utils/extract.py", line 84, in is_extractable
    return tarfile.is_tarfile(path)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/tarfile.py", line 2517, in is_tarfile
    t = open(name)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/tarfile.py", line 1632, in open
    return func(name, "r", fileobj, **kwargs)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/tarfile.py", line 1698, in gzopen
    fileobj = GzipFile(name, mode + "b", compresslevel, fileobj)
  File "~/miniconda3/envs/llama-nol/lib/python3.10/gzip.py", line 174, in __init__
    fileobj = self.myfileobj = builtins.open(filename, mode or 'rb')
PermissionError: [Errno 13] Permission denied: '/path/to/shared-folder/downloads/4e7db366b1ea045d0faa083a2e47ac87326ad8e653f894763b0982c2a1e94078.cc96367835404d4195bf75b2602e6dbbfd2da9288170fc0b2298fc0e376ff52a.py'
```

If I understand this comment from @bhavitvyamalik:

> Yeah sure! Instead of using default `0o644` should I first extract umask of current user and then use `os.umask` on it? We can do it inside `Dataset` class so that all folders/files created during the call use running user's umask

the goal was to infer the `umask` of the user profile, but perhaps I misunderstood and the true solution is to chmod the `cache_files` as @lhoestq suggests above - is that correct?

cc @natolambert @nazneenrajani @edbeeching 

Python files are stored in the modules caches, that you can modify by setting `HF_MODULES_CACHE` as well and set appropriate permissions

They are in different locations because the `HF_MODULES_CACHE` is added to the python path to be able to import the dataset scripts :)

Thanks a lot for the tip @lhoestq ! I didn't know about this extra cache - thanks :)