Bug when trying to open zarr files

[cmdupuis3]

@cisaacstern @rabernat

Charles and I found a bug in trying to write the recipe in pangeo-forge/staged-recipes#239. Running the recipe throws an error, which he realized would also be triggered by running this:

import xarray as xr
import fsspec

file = xr.open_dataset(fsspec.open("https://g-402b74.00888.8540.data.globus.org/eddy/low_res.zarr"), engine="zarr")

which appears to be the current behavior, where the expected behavior would be:

file = xr.open_dataset("https://g-402b74.00888.8540.data.globus.org/eddy/low_res.zarr", engine="zarr")

The error I get is ValueError: Starting with Zarr 2.11.0, stores must be subclasses of BaseStore, if your store exposes the MutableMapping interface wrap it in Zarr.storage.KVStore. Got <OpenFile 'https://g-402b74.00888.8540.data.globus.org/eddy/low_res.zarr'>

[rabernat]

Thanks for opening the issue Chris! I agree this is a bug. We should be able to open zarr files based on their path. This already works in the beam-refactor branch. The question is whether it makes sense to back-port a fix to the main branch.

To answer that, I'd like to get a sense from @cisaacstern of how difficult it would be to deploy a new version of pangeo-forge-recipes to production? If it is very hard to deploy a new version, then it doesn't make sense implementing a fix, because we won't be able to deploy it anyway. If it is easy, then we can do the fix, release a new version, deploy to prod, and get the recipe shipped.

[cisaacstern]

how difficult it would be to deploy a new version of pangeo-forge-recipes to production?

For a non-breaking change to pangeo-forge-recipes, this is easy. It would just require specifying the desired version in this requirements.txt. (pangeo-forge-recipes isn't defined explicitly there at the moment, because it's brought in by pangeo-forge-runner.)

[rabernat]

Ok, so in that case I will sketch out how this fix looks. Basically, you want Zarr URLs to be treated the same way opendata URLs are currently treated: no caching, and no opening with fsspec...instead they should be passed directly through to xarray.open_dataset.

Step 1: Add a Zarr filetype here

pangeo-forge-recipes/pangeo_forge_recipes/patterns.py

Lines 111 to 117 in fe5d1f7

	class FileType(AutoName):
	unknown = auto()
	netcdf3 = auto()
	netcdf4 = auto()
	grib = auto()
	opendap = auto()
	tiff = auto()

Step 2: Add cases for Zarr in pangeo_forge_recipes/recipes/xarray_zarr.py

Basically just search for every instance of FileType.opendap and replace it with FileType.opendap OR FileType.zarr. Something like

non_openable_types = (FileType.opendap, FileType.zarr)
if config.file_pattern.file_type in non_openable_types:
    # special case for Zarr and opendap

Step 3: Verify that it works

Normally this is where we would add tests. And indeed in the beam-refactor, we cover Zarr inputs extensively, e.g.

pangeo-forge-recipes/tests/conftest.py

Lines 233 to 237 in 0e16f15

	@pytest.fixture(scope="session")
	def zarr_local_paths_sequential_1d(daily_xarray_dataset, tmpdir_factory):
	return make_local_paths(
	daily_xarray_dataset, tmpdir_factory, "D", split_up_files_by_day, file_type="zarr"
	)

However, implementing these tests in the main branch would be a major refactor. The test suite is very confusing for a new contributor. Since we are planning to deprecate this code anyway, I am fine with the idea of taking on technical debt by not testing for Zarr.

But we still need some verification that the feature actually works. You could do this by trying to run the recipe from pangeo-forge/staged-recipes#239 locally against your development branch.

[cisaacstern]

@cmdupuis3 would you like to work on this? With Ryan's roadmap above, it should be straightforward, and I'm happy to consult on slack/video if needed.

[cisaacstern]

@cmdupuis3, note that a few of the tests are failing on all PRs to master right now for as-yet unknown reasons (xref #451). So if you do start working on this and notice failing tests, don't necessarily assume you've broken anything. We'll either have to resolve #451, or depending on the timeline, just move forward accepting that certain known test failures will happen that are not related to this work.

[cmdupuis3]

Sounds good, I'll try to get this done by the end of the week.

[rabernat]

Great! The development guide should help you get started.

[cmdupuis3]

Do I need to do anything with this bit?

if self.open_input_with_kerchunk:
    raise ValueError("Can't generate references on opendap inputs")

It seems specific to Opendap, so I'm leaving it alone for now.

[cisaacstern]

It seems specific to Opendap, so I'm leaving it alone for now.

I agree

[cisaacstern]

Closed by #462