use xarray.open_zarr and make aiohttp and s3fs optional #1016

vincentsarago · 2024-10-31T09:53:04Z

This PR does:

remove reference and consolidated options
fall back to open_zarr when not dealing with netcdf files
make aiohttp and s3fs optional dependencies

Note:

we could also make h5netcdf and zarr optional but for this first version I think it's better to at least have something that support both format
in Theory, a user could provider its own opener method (Callable[..., xarray.Dataset]) making all the custom xarray dataset opener optional

abarciauskas-bgse · 2024-10-31T17:30:07Z

This could still work with kerchunk references, do you know? If not I will try to test it. It may not matter if we go completely with icechunk stores (which should have the same access API as vanilla zarr stores) but it would be good to know if we were losing support for kerchunk references.

vincentsarago · 2024-10-31T21:39:23Z

@abarciauskas-bgse good point, I haven't tested but I will 🙏

vincentsarago · 2024-11-04T14:48:49Z

src/titiler/xarray/titiler/xarray/io.py

+            "fo": src_path.replace("reference://", ""),
+            "remote_options": {"anon": anon},
+        }
+        return fsspec.filesystem("reference", **reference_args).get_mapper("")


check if we have a reference:// url or a .json file

vincentsarago · 2024-11-04T15:03:54Z

@abarciauskas-bgse I've added (and tested) back the Kerchunk reference support but I would love a second pair of 👀 to check 🙏

maxrjones · 2024-11-04T15:39:59Z

src/titiler/xarray/titiler/xarray/io.py

+    elif protocol == "reference" or src_path.lower().endswith(".json"):
+        reference_args = {
+            "fo": src_path.replace("reference://", ""),
+            "remote_options": {"anon": anon},
+        }
        return fsspec.filesystem("reference", **reference_args).get_mapper("")


there are at least three ways to store virtual references:

json following kerchunk spec

parquet following kerchunk spec

icechunk spec (currently mostly messagepack)

Do you think the "reference" protocol will be used for all of these in the future, with some automatic differentiation between the storage options? Another option would be to specifically have "kerchunk" and "icechunk" as protocol options, with automatic differentiations between json and parquet based on the file extension.

I guess we could just support reference:// prefix instead of checking the .json

I'm also not sure how to support S3:// referenced files, I'm reading that you can use "remote_protocol": "s3", but we don't have ways to know this!

cc @abarciauskas-bgse

8e10da4

only support reference with reference:// prefix and added support for parquet file (but fastparquet dependency should be installed)

I'm going to try and figure out how to test this locally but in the mean time, here is what I am wondering:

json following kerchunk spec - the current implementation should work for local json references (reference://file.json) but would it work for remotely stored references, e.g. s3://file.json or http://file.json?

parquet following kerchunk spec - files will typically end in .parq or .parquet - again I think this would work for local parquet stores but not sure about remote parquet stores which will be using the s3 or https protocol

@maxrjones have you tested opening an icechunk store with fsspec and passing that as an argument to xarray? I haven't so I am not sure if it will work, but I think we can address icechunk support later.

If we have src_path.lower().endswith(".json") I feel like we should also have .parquet conditional here, but I think this is too coupled with what formats kerchunk is implemented in and we should rely on something else... like a path parameter 😬

in the latest commit I've removed the .json test to only accept reference:// prefix

I guess we could accept things like reference+s3:// 🤷

@abarciauskas-bgse 🤔 I'm not sure we can really support non-local reference file!

FYI, I've tried reference://s3://ds-satellite/netcdf/reference.json and it correctly read the reference file but fails at reading the referenced netcdf (stored on S3 as well). I guess I need to set remote_protocol="s3" somewhere.

@abarciauskas-bgse are we using kerchunk in any production project? to me it seems this need more input definition

We are using it here: https://www.earthdata.nasa.gov/dashboard/data-catalog/combined_CMIP6_daily_GISS-E2-1-G_tas_kerchunk_DEMO (which I believe uses an s3 protocol for the reference file (private bucket) and the netcdfs it points to are in a public s3 bucket) so I know it should work. I can take a closer look later today ...

by the way, I'm on the fence about whether we support kerchunk-style references in this new xarray integration to core titiler, since I think we will be able to use icechunk for reference-style (aka virtual) access in the future. Anyways, kerchunk-style references are still supported by titiler-xarray so we wouldn't have to deprecate https://www.earthdata.nasa.gov/dashboard/data-catalog/combined_CMIP6_daily_GISS-E2-1-G_tas_kerchunk_DEMO right away.

@maxrjones thoughts here?

I can see the motivation for dropping support, since it seems that icechunk is committed to avoiding the fsspec method of using keyword arguments for configuration which is the source of a lot of the complexity here.

Ya, ok, @vincentsarago sorry for the delay in decision here, but lets just drop reference support for now and we can add it back if we decide we need it.

Ideally we could have some documentation about how to easily add it, if people want kerchunk-style reference support with titiler, but I'm not sure at this point how we would do that.

maxrjones

nice!

maxrjones · 2024-11-04T15:41:09Z

src/titiler/xarray/pyproject.toml

+    "zarr",
+    "h5netcdf",


IMO Zarr and h5netcdf should be optional dependencies so people could deploy titiler-xarray with only one option to keep image size down. This may also make it easier to support other HDF5 readers that work better for remote files in the future (e.g., https://github.com/gauteh/hidefix).

Suggested change

"zarr",

"h5netcdf",

maxrjones · 2024-11-04T15:43:20Z

src/titiler/xarray/pyproject.toml

+all = [
+    "s3fs",
+    "aiohttp",
+]


Suggested change

all = [

"s3fs",

"aiohttp",

]

zarr = [

"zarr",

]

hdf5 = [

"h5netcdf",

]

all = [

"s3fs",

"aiohttp",

"zarr",

"hdf5",

]

Suggestion to align with above comment about optional dependencies

@maxrjones I'll take care of this either in the main PR or in another one later

* sketch * add tests * add pyramid tests * remove multiscale option * Update src/titiler/xarray/tests/test_factory.py Co-authored-by: Henry Rodman <[email protected]> * use xarray.open_zarr and make aiohttp and s3fs optional (#1016) * use xarray.open_zarr and make aiohttp and s3fs optional * add support for references * tests prefixed protocol * use tmp_dir for reference * add parquet support * remove kerchunk support * create variable extension * add aiohttp * remove cache layer (#1019) * remove cache layer * Update src/titiler/xarray/README.md Co-authored-by: Aimee Barciauskas <[email protected]> * add tile example --------- Co-authored-by: Aimee Barciauskas <[email protected]> * Update src/titiler/xarray/titiler/xarray/io.py Co-authored-by: Jonas <[email protected]> * Update src/titiler/xarray/titiler/xarray/io.py Co-authored-by: Jonas <[email protected]> * Update src/titiler/xarray/titiler/xarray/io.py Co-authored-by: Jonas <[email protected]> * lint * fix zarr pyramid tests * Update src/titiler/xarray/pyproject.toml Co-authored-by: Max Jones <[email protected]> * refactor dependencies * update docs --------- Co-authored-by: Henry Rodman <[email protected]> Co-authored-by: Aimee Barciauskas <[email protected]> Co-authored-by: Jonas <[email protected]> Co-authored-by: Max Jones <[email protected]>

use xarray.open_zarr and make aiohttp and s3fs optional

3fefc67

vincentsarago requested review from maxrjones and abarciauskas-bgse October 31, 2024 09:53

add support for references

943f5eb

vincentsarago force-pushed the path/use-open-zarr-and-optional-dependencies branch from 07e4beb to 943f5eb Compare November 4, 2024 14:48

vincentsarago commented Nov 4, 2024

View reviewed changes

tests prefixed protocol

3d40b99

This comment was marked as resolved.

Sign in to view

use tmp_dir for reference

8dffefc

maxrjones reviewed Nov 4, 2024

View reviewed changes

maxrjones approved these changes Nov 4, 2024

View reviewed changes

vincentsarago force-pushed the path/use-open-zarr-and-optional-dependencies branch from 8e10da4 to f45ebe4 Compare November 4, 2024 16:42

add parquet support

ea4709c

vincentsarago force-pushed the path/use-open-zarr-and-optional-dependencies branch from f45ebe4 to ea4709c Compare November 4, 2024 16:42

remove kerchunk support

b9b7f30

vincentsarago merged commit d0804ec into feature/add-xarray-package Nov 4, 2024
10 checks passed

vincentsarago deleted the path/use-open-zarr-and-optional-dependencies branch November 4, 2024 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use xarray.open_zarr and make aiohttp and s3fs optional #1016

use xarray.open_zarr and make aiohttp and s3fs optional #1016

vincentsarago commented Oct 31, 2024

abarciauskas-bgse commented Oct 31, 2024

vincentsarago commented Oct 31, 2024

vincentsarago Nov 4, 2024

vincentsarago commented Nov 4, 2024

This comment was marked as resolved.

maxrjones Nov 4, 2024

vincentsarago Nov 4, 2024

vincentsarago Nov 4, 2024

abarciauskas-bgse Nov 4, 2024

vincentsarago Nov 4, 2024

vincentsarago Nov 4, 2024

abarciauskas-bgse Nov 4, 2024

abarciauskas-bgse Nov 4, 2024 •

edited

Loading

maxrjones Nov 4, 2024

abarciauskas-bgse Nov 4, 2024

maxrjones left a comment

maxrjones Nov 4, 2024

maxrjones Nov 4, 2024

vincentsarago Nov 4, 2024

use xarray.open_zarr and make aiohttp and s3fs optional #1016

use xarray.open_zarr and make aiohttp and s3fs optional #1016

Conversation

vincentsarago commented Oct 31, 2024

abarciauskas-bgse commented Oct 31, 2024

vincentsarago commented Oct 31, 2024

Choose a reason for hiding this comment

vincentsarago commented Nov 4, 2024

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarciauskas-bgse Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxrjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarciauskas-bgse Nov 4, 2024 •

edited

Loading