Test ManifestStore against S3 and fix key parsing #507

maxrjones · 2025-03-25T18:34:49Z

This PR test the ManifestStore against S3 using MinIO and fixes a small issue with translating ManifestArray keys to the appropriate key for obstore.get().

Closes Test against S3 using minio #97
Tests added
Tests passing
Full type hint coverage
Changes are documented in docs/releases.rst
New functions/methods are listed in api.rst
New functionality has documentation

TomNicholas

This is cool, but I'm not totally sure I understand what you're intending to test here, or how it relates to the ManifestStore work. Surely dealing with remote object storage is the responsibility of obstore and icechunk?

virtualizarr/tests/test_remote_integration/test_remote_icechunk.py

maxrjones · 2025-03-25T19:43:32Z

This is cool, but I'm not totally sure I understand what you're intending to test here, or how it relates to the ManifestStore work. Surely dealing with remote object storage is the responsibility of obstore and icechunk?

I would like to test that ManifestStore.get() and ManifestStore.list_prefix() return the same expected results for files on both local and object storage. Since ManifestStore requires some URL manipulation, it is possible that we make assumptions valid for local filesystems but not object storage as mentioned as a concern in #490 (comment).

I built this as a separate PR under the assumption that PR scope should be as constrained as possible and that there were some Icechunk tests that would benefit from minio., but didn't pick a good test to start from. I'll modify this PR to test

VirtualiZarr/virtualizarr/tests/test_backend.py

Lines 206 to 232 in 206277f

    
           @requires_network 
        
           @requires_s3fs 
        
           class TestReadFromS3: 
        
               @pytest.mark.parametrize( 
        
                   "indexes", 
        
                   [ 
        
                       None, 
        
                       pytest.param({}, marks=pytest.mark.xfail(reason="not implemented")), 
        
                   ], 
        
                   ids=["None index", "empty dict index"], 
        
               ) 
        
               @parametrize_over_hdf_backends 
        
               def test_anon_read_s3(self, indexes, hdf_backend): 
        
                   """Parameterized tests for empty vs supplied indexes and filetypes.""" 
        
                   # TODO: Switch away from this s3 url after minIO is implemented. 
        
                   fpath = "s3://carbonplan-share/virtualizarr/local.nc" 
        
                   with open_virtual_dataset( 
        
                       fpath, 
        
                       indexes=indexes, 
        
                       reader_options={"storage_options": {"anon": True}}, 
        
                       backend=hdf_backend, 
        
                   ) as vds: 
        
                       assert vds.dims == {"time": 2920, "lat": 25, "lon": 53} 
        
                       assert isinstance(vds["air"].data, ManifestArray) 
        
                       for name in ["time", "lat", "lon"]: 
        
                           assert isinstance(vds[name].data, np.ndarray)

instead, which is explicitly testing for S3 compatibilty.

TomNicholas · 2025-03-25T19:58:38Z

I would like to test that ManifestStore.get() and ManifestStore.list_prefix() return the same expected results for files on both local and object storage.

Thanks for the explanation, that makes a lot more sense.

I just want to avoid the rabbit hole of adding tests to VirtualiZarr that are really just testing that Icechunk does what it says it does.

I built this as a separate PR under the assumption that PR scope should be as constrained as possible

FWIW I would be happy to merge your ManifestStore as it is and iterate on testing that.

maxrjones · 2025-03-25T20:12:23Z

FWIW I would be happy to merge your ManifestStore as it is and iterate on testing that.

Oh wow, that'd make this so much easier!

maxrjones · 2025-03-26T21:05:16Z

virtualizarr/manifests/store.py

+            parsed_key = urlparse(request_key)
+            return StoreRequest(store=stores[key], key=parsed_key.path)


This is the only non-test related change in the PR, which makes the ManifestStore work for both local and object stores

TomNicholas

Nice! This does seem useful.

TomNicholas · 2025-03-27T03:07:58Z

virtualizarr/tests/test_manifests/test_store.py



 @pytest.fixture()
-@requires_obstore


Why does this test not require obstore?

These markers don't make any impact on fixtures, so I removed it. The fixture is generated if any tests using the fixtures are captured by pytest, so the markers should all go on tests rather than fixtures.

TomNicholas · 2025-03-27T03:10:52Z

virtualizarr/tests/test_manifests/test_store.py

+        client_options={"allow_http": True},
+    )
+    filepath = "data.tmp"
+    obs.put(


You only did one put, but now there are 4 chunks in there?

I added a docstring explaining the approach in 874c6df (#507). This is kinda similar to libraries that concurrently compress each chunk (or not for uncompressed data), concatenate them, and then write then to a file in one go. At least, I think that's a thing.

TomNicholas · 2025-03-27T03:12:00Z

virtualizarr/tests/test_manifests/test_store.py

+    @pytest.mark.asyncio
+    @pytest.mark.parametrize(
+        "manifest_store",
+        ["local_store", pytest.param("s3_store", marks=pytest.mark.minio)],


Should the mark be requires_minio?

They're synonymous, but I will re-add requires_minio since it's more descriptive

TomNicholas · 2025-03-27T03:14:59Z

virtualizarr/tests/test_manifests/test_store.py

+            "path": f"s3://{minio_bucket['bucket']}/{filepath}",
+            "offset": 12,
+            "length": 4,
+        },
    }
    manifest = ChunkManifest(entries=chunk_dict)


Is there a way to split this fixture up a bit, to more clearly delineate the creation of data in a (minio) s3-like obstore from the creation of the ManifestStore object which contains the byte references pointing to it?

good call, done in 874c6df (#507)

Setup minio for testing S3 stores

5773879

maxrjones added CI Continuous Integration testing labels Mar 25, 2025

maxrjones temporarily deployed to test-release March 25, 2025 18:35 — with GitHub Actions Inactive

Merge branch 'develop' into minio

a826d66

maxrjones temporarily deployed to test-release March 25, 2025 18:37 — with GitHub Actions Inactive

maxrjones added 2 commits March 25, 2025 14:56

Fix typing

1d1c875

Specify port

cee2cb9

TomNicholas reviewed Mar 25, 2025

View reviewed changes

virtualizarr/tests/test_remote_integration/test_remote_icechunk.py Outdated Show resolved Hide resolved

virtualizarr/tests/test_remote_integration/test_remote_icechunk.py Outdated Show resolved Hide resolved

Run minio tests without pytest-xdist

4d6a8f5

Merge branch 'develop' into minio

8f9f0fd

maxrjones changed the title ~~Setup minio for testing S3 stores~~ WIP: Setup minio for testing S3 stores Mar 25, 2025

maxrjones marked this pull request as draft March 25, 2025 21:37

Add WIP on obstore S3 tests

c12252e

maxrjones changed the title ~~WIP: Setup minio for testing S3 stores~~ WIP: Setup MinIO for testing S3 stores Mar 26, 2025

maxrjones temporarily deployed to test-release March 26, 2025 17:17 — with GitHub Actions Inactive

Revert change to run-tests command

f149885

maxrjones temporarily deployed to test-release March 26, 2025 17:19 — with GitHub Actions Inactive

Separate env

04d50d0

maxrjones temporarily deployed to test-release March 26, 2025 18:12 — with GitHub Actions Inactive

maxrjones added 2 commits March 26, 2025 17:01

Update pytest marks

6d63ab3

Fix key manipulation

ecd722f

maxrjones changed the title ~~WIP: Setup MinIO for testing S3 stores~~ Test ManifestStore against S3 and fix key parsing Mar 26, 2025

maxrjones added the bug Something isn't working label Mar 26, 2025

maxrjones temporarily deployed to test-release March 26, 2025 21:02 — with GitHub Actions Inactive

Revert conftest changes

65f6374

maxrjones temporarily deployed to test-release March 26, 2025 21:04 — with GitHub Actions Inactive

Revert kerchunk test change

220fd50

maxrjones commented Mar 26, 2025

View reviewed changes

maxrjones temporarily deployed to test-release March 26, 2025 21:05 — with GitHub Actions Inactive

maxrjones marked this pull request as ready for review March 26, 2025 21:05

mypy ignore MinIO

cb566e6

maxrjones temporarily deployed to test-release March 26, 2025 21:07 — with GitHub Actions Inactive

maxrjones requested a review from TomNicholas March 26, 2025 21:15

TomNicholas approved these changes Mar 27, 2025

View reviewed changes

maxrjones added 2 commits March 27, 2025 15:34

Separate fixtures and add docstring

874c6df

Update pytest markers

6a4cda4

maxrjones temporarily deployed to test-release March 27, 2025 19:43 — with GitHub Actions Inactive

maxrjones merged commit fd2ccf3 into zarr-developers:develop Mar 27, 2025
11 checks passed

maxrjones deleted the minio branch March 27, 2025 20:00

abarciauskas-bgse mentioned this pull request Apr 1, 2025

Add VirtualiZarr reader for async-tiff NASA-IMPACT/veda-odd#88

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test ManifestStore against S3 and fix key parsing #507

Test ManifestStore against S3 and fix key parsing #507

maxrjones commented Mar 25, 2025 •

edited

Loading

TomNicholas left a comment

maxrjones commented Mar 25, 2025

TomNicholas commented Mar 25, 2025 •

edited

Loading

maxrjones commented Mar 25, 2025

maxrjones Mar 26, 2025

TomNicholas left a comment

TomNicholas Mar 27, 2025

maxrjones Mar 27, 2025

TomNicholas Mar 27, 2025

maxrjones Mar 27, 2025

TomNicholas Mar 27, 2025

maxrjones Mar 27, 2025

TomNicholas Mar 27, 2025

maxrjones Mar 27, 2025

		parsed_key = urlparse(request_key)
		return StoreRequest(store=stores[key], key=parsed_key.path)



		@pytest.fixture()
		@requires_obstore

Test ManifestStore against S3 and fix key parsing #507

Test ManifestStore against S3 and fix key parsing #507

Conversation

maxrjones commented Mar 25, 2025 • edited Loading

TomNicholas left a comment

Choose a reason for hiding this comment

maxrjones commented Mar 25, 2025

TomNicholas commented Mar 25, 2025 • edited Loading

maxrjones commented Mar 25, 2025

Choose a reason for hiding this comment

TomNicholas 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

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

maxrjones commented Mar 25, 2025 •

edited

Loading

TomNicholas commented Mar 25, 2025 •

edited

Loading