From 1137fcd3a0819d77854e1caf7460adae527c14d0 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 18 Apr 2024 14:50:59 -0700 Subject: [PATCH 1/7] Add sdupport for r- read mode to read without consolidated metadata --- src/hdmf_zarr/backend.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hdmf_zarr/backend.py b/src/hdmf_zarr/backend.py index 6ae27a25..d41453b2 100644 --- a/src/hdmf_zarr/backend.py +++ b/src/hdmf_zarr/backend.py @@ -88,7 +88,8 @@ def can_read(path): 'doc': 'the path to the Zarr file or a supported Zarr store'}, {'name': 'manager', 'type': BuildManager, 'doc': 'the BuildManager to use for I/O', 'default': None}, {'name': 'mode', 'type': str, - 'doc': 'the mode to open the Zarr file with, one of ("w", "r", "r+", "a", "w-")'}, + 'doc': 'the mode to open the Zarr file with, one of ("w", "r", "r+", "a", "w-") ' + 'the mode r- is used to force open without consolidated metadata in read only mode.'}, {'name': 'synchronizer', 'type': (zarr.ProcessSynchronizer, zarr.ThreadSynchronizer, bool), 'doc': 'Zarr synchronizer to use for parallel I/O. If set to True a ProcessSynchronizer is used.', 'default': None}, @@ -161,7 +162,7 @@ def open(self): # As a result, when in other modes, the file will not use consolidated metadata. if self.__mode not in ['r', 'r+']: self.__file = zarr.open(store=self.path, - mode=self.__mode, + mode=self.__mode if self.__mode is not 'r-' else 'r', synchronizer=self.__synchronizer, storage_options=self.__storage_options) else: From c368f04abbcd745dc0992bfd96ddd2bf12a6e8da Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:18:20 -0700 Subject: [PATCH 2/7] Updated CHANGELOG for #183 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f1bb7c5..ad27926b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## 0.7.0 (Upcoming) ### Enhancements * Added support for python 3.12. @mavaylon1 [#172](https://github.com/hdmf-dev/hdmf-zarr/pull/172) +* Added support for forcing read of files without consolidated metadata using `mode=r-` in `ZarrIO`. @oruebel [#183](https://github.com/hdmf-dev/hdmf-zarr/pull/183) ### Docs * Removed `linkable` from the documentation to keep in line with `hdmf-schema-language`. @mavaylon1 [#180](https://github.com/hdmf-dev/hdmf-zarr/pull/180) From 75181d6a4a57fff864f57df6f303adffdb8ba0ad Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:29:38 -0700 Subject: [PATCH 3/7] Add error check and fix bug in ZarrIO.__open_consolidate --- src/hdmf_zarr/backend.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/hdmf_zarr/backend.py b/src/hdmf_zarr/backend.py index d41453b2..366b9ac1 100644 --- a/src/hdmf_zarr/backend.py +++ b/src/hdmf_zarr/backend.py @@ -162,7 +162,7 @@ def open(self): # As a result, when in other modes, the file will not use consolidated metadata. if self.__mode not in ['r', 'r+']: self.__file = zarr.open(store=self.path, - mode=self.__mode if self.__mode is not 'r-' else 'r', + mode=self.__mode if self.__mode != 'r-' else 'r', synchronizer=self.__synchronizer, storage_options=self.__storage_options) else: @@ -479,6 +479,9 @@ def __open_file_consolidated(self, This method will check to see if the metadata has been consolidated. If so, use open_consolidated. """ + # This check is just a safeguard for possible errors in the future. But this should never happen + if mode == 'r-': + raise ValueError('Mode r- not allowed for reading with consolidated metadata') # self.path can be both a string or a one of the `SUPPORTED_ZARR_STORES`. if isinstance(self.path, str): path = self.path @@ -491,10 +494,10 @@ def __open_file_consolidated(self, synchronizer=synchronizer, storage_options=storage_options) else: - return zarr.open(store=self.path, - mode=self.__mode, - synchronizer=self.__synchronizer, - storage_options=self.__storage_options) + return zarr.open(store=store, + mode=mode, + synchronizer=synchronizer, + storage_options=storage_options) @docval({'name': 'parent', 'type': Group, 'doc': 'the parent Zarr object'}, {'name': 'builder', 'type': GroupBuilder, 'doc': 'the GroupBuilder to write'}, @@ -724,7 +727,9 @@ def resolve_ref(self, zarr_ref): else: target_name = ROOT_NAME - target_zarr_obj = self.__open_file_consolidated(source_file, mode='r', storage_options=self.__storage_options) + target_zarr_obj = self.__open_file_consolidated(store=source_file, + mode='r', + storage_options=self.__storage_options) if object_path is not None: try: target_zarr_obj = target_zarr_obj[object_path] From 94bbc6706d32ac53afe9664fdbb0964769d9627b Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:30:20 -0700 Subject: [PATCH 4/7] Avoid error on clean-up if the file was not created in a test --- tests/unit/base_tests_zarrio.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/base_tests_zarrio.py b/tests/unit/base_tests_zarrio.py index d142499d..b63bb1cf 100644 --- a/tests/unit/base_tests_zarrio.py +++ b/tests/unit/base_tests_zarrio.py @@ -96,7 +96,8 @@ def setUp(self): self.store = "tests/unit/test_io.zarr" def tearDown(self): - shutil.rmtree(self.store) + if os.path.exists(self.store): + shutil.rmtree(self.store) def createReferenceBuilder(self): data_1 = np.arange(100, 200, 10).reshape(2, 5) From 7a89fc5b35842a840f0b419f8ee808691eaf414b Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:30:40 -0700 Subject: [PATCH 5/7] Add unit tests for r- read mode --- tests/unit/test_zarrio.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/unit/test_zarrio.py b/tests/unit/test_zarrio.py index 2af9bcef..081706a8 100644 --- a/tests/unit/test_zarrio.py +++ b/tests/unit/test_zarrio.py @@ -152,3 +152,33 @@ def test_get_store_path_deep(self): path = ZarrIO._ZarrIO__get_store_path(store) expected_path = os.path.normpath(os.path.join(CUR_DIR, 'test_io.zarr')) self.assertEqual(path, expected_path) + + def test_force_open_without_consolidated(self): + """Test that read-mode -r forces a regular read with mode r""" + self.create_zarr(consolidate_metadata=True) + # Confirm that opening the file 'r' mode indeed uses the consolidated metadata + with ZarrIO(self.store, mode='r') as read_io: + read_io.open() + self.assertIsInstance(read_io.file.store, zarr.storage.ConsolidatedMetadataStore) + # Confirm that opening the file IN 'r-' mode indeed forces a regular open without consolidated metadata + with ZarrIO(self.store, mode='r-') as read_io: + read_io.open() + self.assertIsInstance(read_io.file.store, zarr.storage.DirectoryStore) + + def test_force_open_without_consolidated_fails(self): + """ + Test that we indeed can't use '_ZarrIO__open_file_consolidated' function in r- read mode, which + is used to force read without consolidated metadata. + """ + self.create_zarr(consolidate_metadata=True) + with ZarrIO(self.store, mode='r') as read_io: + # Check that using 'r-' fails + msg = 'Mode r- not allowed for reading with consolidated metadata' + with self.assertRaisesWith(ValueError, msg): + read_io._ZarrIO__open_file_consolidated(store=self.store, mode='r-') + # Check that using 'r' does not fail + try: + read_io._ZarrIO__open_file_consolidated(store=self.store, mode='r') + except ValueError as e: + self.fail("ZarrIO.__open_file_consolidated raised an unexpected ValueError: {}".format(e)) + From 944a0720b15f554910c27ddcd6b43d64fa301b41 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:30:51 -0700 Subject: [PATCH 6/7] Update CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad27926b..6fde301c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ ### Docs * Removed `linkable` from the documentation to keep in line with `hdmf-schema-language`. @mavaylon1 [#180](https://github.com/hdmf-dev/hdmf-zarr/pull/180) +### Internal Fixes +* Fixed minor bug where `ZarrIO.__open_file_consolidated` used properties of `ZarrIO` instead of the provided input parameters. @oruebel [#183](https://github.com/hdmf-dev/hdmf-zarr/pull/183) + ## 0.6.0 (February 21, 2024) ### Enhancements From e28904e1af07b523f1fb6567634fe3496d50783a Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 25 Apr 2024 14:42:54 -0700 Subject: [PATCH 7/7] Move conditional mode check to a variable assignement --- src/hdmf_zarr/backend.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hdmf_zarr/backend.py b/src/hdmf_zarr/backend.py index 366b9ac1..c1e82cd6 100644 --- a/src/hdmf_zarr/backend.py +++ b/src/hdmf_zarr/backend.py @@ -161,8 +161,11 @@ def open(self): # Within zarr, open_consolidated only allows the mode to be 'r' or 'r+'. # As a result, when in other modes, the file will not use consolidated metadata. if self.__mode not in ['r', 'r+']: + # r- is only an internal mode in ZarrIO to force the use of regular open. For Zarr we need to + # use the regular mode r when r- is specified + mode_to_use = self.__mode if self.__mode != 'r-' else 'r' self.__file = zarr.open(store=self.path, - mode=self.__mode if self.__mode != 'r-' else 'r', + mode=mode_to_use, synchronizer=self.__synchronizer, storage_options=self.__storage_options) else: