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

Fix small open_virtual_dataset bugs #923

Merged
merged 6 commits into from
Jan 17, 2025

Conversation

ayushnag
Copy link
Collaborator

@ayushnag ayushnag commented Jan 8, 2025


📚 Documentation preview 📚: https://earthaccess--923.org.readthedocs.build/en/923/

Copy link

github-actions bot commented Jan 8, 2025

Binder 👈 Launch a binder notebook on this branch for commit 8f9d27e

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit d5ca8c7

Binder 👈 Launch a binder notebook on this branch for commit 63f36b6

Binder 👈 Launch a binder notebook on this branch for commit 3d3a279

Binder 👈 Launch a binder notebook on this branch for commit 529b5ca

Binder 👈 Launch a binder notebook on this branch for commit f39b151

@ayushnag ayushnag changed the title Minor open_virtual_dataset results bugs Fix small open_virtual_dataset results bugs Jan 8, 2025
@ayushnag ayushnag changed the title Fix small open_virtual_dataset results bugs Fix small open_virtual_dataset bugs Jan 8, 2025
@ayushnag
Copy link
Collaborator Author

ayushnag commented Jan 8, 2025

Note that this PR changes the dmrpp tests to no longer verify the result of open_virtual_dataset since that is checked in virtualizarr. The tests here now check that the expected dmrpp files are found, parsed, and can be loaded.

betolink
betolink previously approved these changes Jan 16, 2025
Copy link
Member

@betolink betolink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, sorry for the delay. Is this the last thing we need before adding example notebooks and releasing 0.13.0? cc @ayushnag

@betolink betolink self-requested a review January 16, 2025 17:21
@betolink
Copy link
Member

oops, looks like there are some failing integration tests. e.g.

tests/integration/test_virtualizarr.py::test_open_virtual_dataset[AVHRR_OI-NCEI-L4-GLOB-v2.1] 

I wonder if it's related to JPL's outage. I'll debug locally.

@ayushnag
Copy link
Collaborator Author

@betolink Thanks for reviewing and pushing the fixes! I believe zarr v3 doesn't break the parser, v3 is not yet compatible with kerchunk. However, kerchunk is needed for the load=True param so it is ok to pin to v2. However, we will have to solve this problem soon since virtualizarr will also soon require zarr v3. The alternatives are to use an icechunk in-memory store to load the data or use a Zarr store designed for reading manifests like the one suggested here.

@betolink
Copy link
Member

Ahhh good to know! ok so let's get this merged and then work on the notebooks so we can have a virtualizarr release next week! @ayushnag

@betolink betolink merged commit 4d0721d into nsidc:main Jan 17, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] incorrect typing on get_s3_filesystem has led open_virtual_mfdataset astray
2 participants