-
-
Notifications
You must be signed in to change notification settings - Fork 330
Use donfig for V3 configuration #1855
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
Conversation
@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. |
-- a few todos remain
There was a problem hiding this 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:
- 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 attributeAsyncArray.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
order
does not make sense to include in theArrayMetadata
. 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?
There was a problem hiding this 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!
src/zarr/codecs/sharding.py
Outdated
@@ -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!!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.
TODO: