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

Use donfig for V3 configuration #1855

Merged
merged 9 commits into from
May 10, 2024
Merged

Conversation

maxrjones
Copy link
Member

@maxrjones maxrjones commented May 8, 2024

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrz normanrz mentioned this pull request May 8, 2024
6 tasks
@jhamman jhamman added the V3 Related to compatibility with V3 spec label May 8, 2024
@jhamman jhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones
Copy link
Member Author

maxrjones commented May 9, 2024

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array = AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1 = array.getitem(...)
    config.set({'array.order': 'F'}}
    a2 = array.getitem(...)
    assert a1.order == a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

src/zarr/array.py Show resolved Hide resolved
src/zarr/array.py Show resolved Hide resolved
src/zarr/metadata.py Show resolved Hide resolved
Copy link
Contributor

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

@@ -631,6 +621,7 @@ def _get_index_chunk_spec(self, chunks_per_shard: ChunkCoords) -> ArraySpec:
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
order="C", # TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhamman jhamman changed the title WIP: Use donfig for V3 configuration Use donfig for V3 configuration May 10, 2024
@pep8speaks
Copy link

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman jhamman merged commit 5bb3375 into zarr-developers:v3 May 10, 2024
18 checks passed
@maxrjones maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V3 Related to compatibility with V3 spec
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants