Skip to content
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

Bug: Using GCS or S3, FSStore doesn't pass check and create kwargs to FSSPEC's get_mapper() #775

Open
tasansal opened this issue Jun 15, 2021 · 7 comments
Labels
good-first-issue Good place to get started as a new contributor. help wanted Issue could use help from someone with familiarity on the topic platform-specific Issue appears to be related to a particular platform/OS

Comments

@tasansal
Copy link
Contributor

tasansal commented Jun 15, 2021

I think this is a bug, if everyone agrees I can fix it and do a PR.

Minimal, reproducible code sample, a copy-pastable example if possible

import zarr

# Works fine with this as expected
url = '~/my_root.zarr'

# Doesn't work with:
url = r'gcs://my_bucket/my_root.zarr'
# or
url = r's3://my_bucket/my_root.zarr'

storage_options = {'key': my_key, 'secret': my_secret, 'token': my_token}  # Example for s3
store = zarr.storage.FSStore(url, check=True, create=True, **storage_options)
zarr.open(store, 'w')

Problem description

The above code fails because the FSMap created and used by FSStore did not receive the check and create kwargs (Traceback at the bottom of the post).

The check parameter is used to verify write access to remote stores.
The create function creates the root object/directory if it doesn't exist, so it can check for write access

The fsspec.mapping.get_mapper used in zarr.storage.FSStore expects explicit kwargs for check and create to send FSMap and passes storage_options aka. other kwargs to ONLY to url_to_fs (here). The FSMap call here expects the check and create kwargs, which explicitly have to be passed to get_mapper.

The issue can be circumvented by dropping the write access checking, but that may not be always the case user wants.

The ideal fix is to make zarr.storage.FSStore to expect check and create kwargs; and default to False, just like FSMap.

Version and installation information

Please provide the following:

  • zarr.__version__ 2.8.1
  • numcodecs.__version__ 0.7.3
  • Python 3.9.5
  • Linux x86_64
  • installed via conda

Traceback:

Traceback (most recent call last):
  File "/home/my_user/develop/my_library/benchmarks/test.py", line 5, in <module>
    store = zarr.storage.FSStore(url, check=True, create=True, **storage_options)
  File "/home/my_user/miniconda3/envs/my_library/lib/python3.9/site-packages/zarr/storage.py", line 1052, in __init__
    self.map = fsspec.get_mapper(url, **storage_options)
  File "/home/my_user/miniconda3/envs/my_library/lib/python3.9/site-packages/fsspec/mapping.py", line 224, in get_mapper
    return FSMap(root, fs, check, create, missing_exceptions=missing_exceptions)
  File "/home/my_user/miniconda3/envs/my_library/lib/python3.9/site-packages/fsspec/mapping.py", line 52, in __init__
    raise ValueError(
ValueError: Path my_bucket/my_root.zarr does not exist. Create  with the ``create=True`` keyword
@martindurant
Copy link
Member

Yes, please fix to pass the kwargs!

@tasansal
Copy link
Contributor Author

@martindurant, Created PR #814 to handle this issue.

@rabernat
Copy link
Contributor

rabernat commented Aug 23, 2021

I am reviewing #814 and wanted to follow up with a question here.

Isn't this a bug with fsspec? The fact that this works with one FSSpec implementation (LocalFileSystem) but not others (gcsfs, s3fs) suggests a problem with those implementations. Working around that bug in zarr-python creates technical debt here.

Is there an upstream fix possible.

@tasansal
Copy link
Contributor Author

@rabernat

Let me verify that it still works as expected on a LocalFileSystem when write-permission is not enabled.

If it works as expected, I agree, we should probably put a fix in fsspec.

@joshmoore
Copy link
Member

Can this issue be closed now?

@martindurant
Copy link
Member

We should probably have tests against this behaviour to be sure, but I suspect everything now works as expected.

@tasansal
Copy link
Contributor Author

tasansal commented Dec 2, 2022

I did a PR to fix this in #814, but it was closed to be handled in fsspec. I haven't done a PR there, has anyone else fixed this?

@joshmoore joshmoore added help wanted Issue could use help from someone with familiarity on the topic good-first-issue Good place to get started as a new contributor. platform-specific Issue appears to be related to a particular platform/OS labels Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good place to get started as a new contributor. help wanted Issue could use help from someone with familiarity on the topic platform-specific Issue appears to be related to a particular platform/OS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants