Feature/recursive members

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.

[TomAugspurger]

(This currently depends on #2117. I'll rebase once that's merged to v3)

This PR adds a recursive=True flag to Group.members, for recursively
listing the members of some hierarchy. The default is unchanged (just list immediate children).

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

This is useful for Consolidated Metadata, which needs to recursively
inspect children. IMO, it's useful (and simple) enough to include
in the public API.

Closes#2119

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[TomAugspurger]

#2119 has some information on the failed hypothesis test. I think it's real.

Edit: 8bd52d0 hopefully fixes that.

[TomAugspurger]

I'm pretty stumped by the failure at https://github.com/zarr-developers/zarr-python/actions/runs/10549846136/job/29225363145?pr=2118#step:6:374

The issue comes from a RuntimeError in numpy when doing something like

np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)

The actual array constructed by hypothesis is a bit different though. It's more like:

In [23]: np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f")[0]
Out[23]: np.complex128(nanj)

The dtype and repr appear to be the same, but apparently they're different internally. Not sure how yet.

Finally, something in the test runner appears to elevate the RuntimeWarning to an error:

# inside the test runarray([False])
(Pdb) np.array([np.complex128(0.0, np.nan)]) ==np.complex128(0)
array([False])
(Pdb) np.ndarray((1,), np.dtype("complex128"), b"\x00\x00\x00\x00\x00\x00\x00\x00\xfd\x7f\xbaUFJ\xf0\x7f") ==np.complex128(0)
***RuntimeWarning: invalidvalueencounteredinequal

[TomAugspurger]

I'm partly giving up on that second hypothesis failure. It's possible that there's still a bug in Buffer.all_equal at

zarr-python/src/zarr/core/buffer.py

Lines 429 to 430 in 681f286

	defall_equal(self, other: Any) ->bool:
	returnbool((self._data==other).all())

not handling NaN like other values. But for this PR, we're able to work around it by filtering out that RuntimeWarning.

This should be ready for review now.

[jhamman]

This includes a breaking change to Group.members and Group.nmembers. Previously, these were properties, while AsyncGroup.members / nmembers were functions. I've changed the sync versions to align with the async versions (which is needed to support this keyword).

Agreed that this is useful enough to justify this change.

[jhamman]

@dcherian - would you mind looking into this?

[TomAugspurger]

It's a minefield 😬 The deepest I got was hypothesis.extra.numpy:ArrayStrategy.set_element. Something about the sequence of operations and the data passed into the ndarray made it "weird", such that array == np.complex128(0.0) raised an InvalidComparision warning. I'm not sure how to interpret the bytes at #2118 (comment).

[d-v-b]

this looks good to me! my one question is whether there would be any utility in using an integer to limit the recursion depth to a desired number of hierarchy levels, while preserving the option to traverse the entire group. e.g., the value of 1 would denote gathering immediate sub-arrays and sub-groups, 2 would recurse once, and -1 would gather the whole hierarchy. I did this over in pydantic-zarr and it has worked well so far. Without this functionality, we are basically forcing users with very deep zarr hierarchies to choose between expensive IO (recursive=True) and writing their own semi-recursive implementations (manually calling members N times).

[TomAugspurger]

I was wondering about a limit-style argument too. I'll go ahead and implement that.

[TomAugspurger]

I opted to implement this as a max_depth: int | None keyword, which replaces recursive=True. By setting max_depth=-1 or max_depth=None you get the same behavior as recursive=True. A positive integer considers just that depth. The default of max_depth=0 is the same as the existing behavior on main of just considering immediate children.

[d-v-b]

thanks @TomAugspurger! what's the reasoning for specifying the same behavior with two distinct values -- max_depth=-1 and max_depth=None? Personally I would be fine with XOR on these options, and it would make things marginally simpler. But maybe there's an advantage to your current approach that I'm not considering.

[TomAugspurger]

Purely for user convenience. Neither max_depth=-1 nor max_depth=None sounded obviously more natural to express "no depth".

If I had to choose, I'd say max_depth=None. Do you have a preference?

[d-v-b]

If I had to choose, I'd say max_depth=None. Do you have a preference?

I like this too!

[d-v-b]

this looks good, pre-commit is red but I don't think that failure is related to content in the PR. If you're all done here @TomAugspurger then I am happy to merge

[TomAugspurger]

Yeah, this should be all set.

The pre-commit errors are at #2131, I think from a new version of NumPy. I'll look into them today.