Make zarrcreate create intermediate zarr groups and work with relative paths #91

krisfed · 2025-05-14T09:20:53Z

According to Zarr spec:

If the user requests an array to be created under some logical path, then groups MUST also be created at all ancestor paths. E.g., if the user requests array creation at path “foo/bar/baz” then groups must be created at path “foo/bar”, path “foo”, and the root of the store, if they don’t already exist.

In this pull request I try to make zarrcreate adhere to this. It is a bit tricky to know what the root of the store is, so for now I just assume that any new directories that need to be created as part of thezarrcreate call are part of the store and need to be made into zarr groups.

For example, if you are running the following command:

>> zarrcreate("/Users/jsmith/Documents/myfile.zarr/A/B/C", [10,10])

and the path "/Users/jsmith/Documents/" already exists, but "myfile.zarr/A/B/C" directories don't yet exist, then the created Zarr groups will be at:

/Users/jsmith/Documents/myfile.zarr
/Users/jsmith/Documents/myfile.zarr/A
/Users/jsmith/Documents/myfile.zarr/A/B

It might make sense to add a zarrcreategroup function at a later point too, but not doing it yet in this pull request.

I also tried to make zarrcreate work with relative paths like "../myfile.zarr/"

Closes #87
Closes #57

… paths

abhibaruah · 2025-05-14T13:03:25Z

test/tZarrCreate.m

+            zarrcreate(arrayPath, testcase.ArrSize);
+            [groupPath, ~, ~] = fileparts(arrayPath);
+
+            testcase.verifyTrue(isfile(fullfile(groupPath, ".zgroup")),...


Are we doing this check for both A and B?

I think in this case B will be a Zarr array (the last element of the path we are passing into zarrcreate).
Not sure if we need a test for a more nested case like fullfile(testcase.ArrPathWrite, "A", "B", "C")

abhibaruah · 2025-05-14T13:04:02Z

test/tZarrCreate.m

+            zarrcreate(arrayPath, testcase.ArrSize);
+            [groupPath, ~, ~] = fileparts(arrayPath);
+
+            testcase.verifyTrue(isfile(fullfile(groupPath, ".zgroup")),...


Did you check if this behaves as expected for a Zarr file hosted on an S3 bucket?

I did a basic check (see comment here: https://github.com/mathworks/MATLAB-support-for-Zarr-files/pull/91/files#r2089009441 ). Let me know if anything else needs to be tried! Wish we had an easier way to run automated tests with S3 locations

abhibaruah · 2025-05-14T13:06:47Z

Zarr.m

@@ -56,6 +56,122 @@
                py.importlib.reload(zarrModule);
            end
        end
+
+        function isZarray = isZarrArray(path)


Should these methods also be made protected to limit access to only the class/object of the class?

That's a good point.. I was thinking at some point we might want to expose some of them (esp createGroup()) through a separate function or move them elsewhere all together. If no big flag, I think we can keep it as is for now?

abhibaruah · 2025-05-14T13:11:47Z

Zarr.m

+                path (1,1) string
+            end
+
+            if path == ""


Not sure I understand this. An empty string must not be allowed, right?

So this function works recursively - it checks if a given path is resolvable to an absolute path, and if not, then it calls itself on the parent folder (until it finds a resolvable absolute path). But if you start with a relative path (say "myzarr/A/B") where none of the folders exist yet, none of "myzarr/A/B", "myzarr/A", or "myzarr" will resolve to an absolute path, so the recursion will end up with a parent folder that is "". At that point, we know that the intention is that "myzarr/A/B" folders will be created at the current location (pwd), so the full path should be pwd + "myzarr/A/B"

abhibaruah · 2025-05-14T13:14:41Z

Zarr.m

+            end
+
+            % See if the parent path exist. Continue recursing until an
+            % exsiting parent path is found


Typo - existing

Nice catch, fixing it

abhibaruah · 2025-05-14T13:17:38Z

Zarr.m

+                path (1,1) string
+            end
+
+            if isfolder(path)


Can you check if this works for S3?

So I tried a simple workflow like this, but let me know if there is something specific I should check:

>> s3ParentFolder = "s3://<...reducted...>/" >> zarrcreate(s3ParentFolder + "myzarr.zarr/A/B/C", [100,100]) >> d = zarrread(s3ParentFolder + "myzarr.zarr/A/B/C"); >> info = zarrinfo(s3ParentFolder + "myzarr.zarr") info = [struct](matlab:helpPopup('struct')) with fields: zarr_format: '2' node_type: 'group' >> info = zarrinfo(s3ParentFolder + "myzarr.zarr/A") info = [struct](matlab:helpPopup('struct')) with fields: zarr_format: '2' node_type: 'group' >> info = zarrinfo(s3ParentFolder + "myzarr.zarr/A/B") info = [struct](matlab:helpPopup('struct')) with fields: zarr_format: '2' node_type: 'group' >> info = zarrinfo(s3ParentFolder + "myzarr.zarr/A/B/C") info = [struct](matlab:helpPopup('struct')) with fields: chunks: [2×1 double] compressor: [] dimension_separator: '.' dtype: '<f8' fill_value: [] filters: [] order: 'C' shape: [2×1 double] zarr_format: 2 node_type: 'array' >> rmdir(s3ParentFolder + "myzarr.zarr", 's')

I have also tried this method separately too:

>> existParent = Zarr.getExistingParentFolder(s3ParentFolder + "myzarr.zarr/A/B/C"); >> existParent + "/" == s3ParentFolder ans = [logical](matlab:helpPopup('logical')) 1 >>

abhibaruah · 2025-05-14T13:22:54Z

Zarr.m

+
+            % if new directories were created as part of creating a
+            % Zarr array, we need to make them into Zarr groups.
+            newDirs = extractAfter(obj.Path, existingParentPath);


What if newDirs is empty?

Then the newGroups will also be empty and we will skip creating Zarr groups because of the if statement that checks for empty newGroups.

>> [newGroups, ~,~] = fileparts("") newGroups = ""

abhibaruah · 2025-05-14T13:30:44Z

Zarr.m

@@ -144,12 +262,27 @@ function create(obj, dtype, data_size, chunk_size, fillvalue, compression)
                obj.FillValue = cast(fillvalue, obj.MatlabDatatype);
            end

+            % see how much of the provided path exists already 
+            existingParentPath = Zarr.getExistingParentFolder(obj.Path);
+
            % The Python function returns the Tensorstore schema, but we
            % do not use it for anything at the moment.
            obj.TensorstoreSchema = py.ZarrPy.createZarr(obj.KVStoreSchema, py.numpy.array(obj.DsetSize),...


So, you are identifying the folders to be created before createZarr and after the creation, you are populating the groups with .zgroup file. Smart!!
Neat use of recursion!!

My only concern would be to do manual testing on S3 buckets (in case you havent done already)

Thanks! :)
It was the best way I could figure out without having to undo folder creation error cases... Still a bit clunky that we have to do it manually. Based on Zarr spec, I almost want to say it is a bug in tensorstore that it doesn't create intermediate zarr groups

abhibaruah · 2025-05-14T13:37:26Z

Zarr.m

+
+        function createGroup(pathToGroup)
+            % Create a Zarr group including creating the directory (if
+            % needed) and the .zrgroup file. Assumes the parent directory


Typo - .zgroup

Nice catch! Fixing it

codecov · 2025-05-14T14:31:52Z

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.66%. Comparing base (046f9c0) to head (ea86b5e).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
Zarr.m	89.74%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   85.98%   86.66%   +0.67%     
==========================================
  Files           7        7              
  Lines         157      195      +38     
==========================================
+ Hits          135      169      +34     
- Misses         22       26       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

krisfed · 2025-05-14T14:34:45Z

zarrcreate.m

-% intermediate groups that do not already exist. If FILEPATH exists
-% already, the contents are overwritten.
+% If FILEPATH is a full path name, the function creates all intermediate
+% directories that do not already exist and makes them into Zarr groups. If


Wasn't sure about making this change in the doc... Let me know what you think! Does this also need to be in-sync with readme?

jm9176

Looks good to me. Please feel free to create test tasks if you were unable to add any tests.

Make zarrcreate create intermediate zgroups and work with .. relative…

ca47715

… paths

krisfed requested review from jm9176 and abhibaruah May 14, 2025 09:20

krisfed marked this pull request as ready for review May 14, 2025 09:23

abhibaruah reviewed May 14, 2025

View reviewed changes

abhibaruah approved these changes May 14, 2025

View reviewed changes

Fix comment typo

8cb87ad

Fix another comment typo

ea86b5e

krisfed commented May 14, 2025

View reviewed changes

jm9176 approved these changes May 15, 2025

View reviewed changes

krisfed merged commit eb5dde8 into main May 16, 2025
5 checks passed

krisfed deleted the test-branch8 branch May 16, 2025 10:43

Make zarrcreate create intermediate zarr groups and work with relative paths #91

Make zarrcreate create intermediate zarr groups and work with relative paths #91

Uh oh!

Conversation

krisfed commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jm9176 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 14, 2025 •

edited

Loading