Conversation
|
Thanks! I can see the rationale of this change for the rectilinear chunk grid. However, I would favor keeping the strict requirement for regular outer chunk grids. That way, implementations that don't implement the rectilinear grid won't need to change. |
I'm interested to hear more perspectives here. Personally, I think this change is very useful with regularly gridded arrays, and irregular grids just make the problem far more apparent. For example, say a chunk shape is constrained by a sensor or data pipeline and must be Would it be sufficient in your view to recommend that implementations that add support for this issue a warning that it will not be supported by impls that only support version 1.0 of the sharding codec? |
|
+1 to this change, it seems like a logical extension of how we currently handle the shape / chunk grid relationship for the regular chunk grid. |
There was a problem hiding this comment.
@LDeakin +1 from me. I am in favor of this.
For context we run into these restrictions being rather annoying when dealing with acquisitions from microscopes. We pick chunk and shard shapes for read/write performance and reducing inode count on our HPCs
Since the shard shape has to be a multiple of the inner chunk shape, it rounds up past the array bound. Part of the shard then sits outside the array, so no write ever covers it and every write becomes a RMW. Usually that just costs performance, but it also means we can't repair a half-written shard by rewriting it, since the rewrite has to read the damaged shard first.
We can pick an inner chunk that divides the extent, but then the prime factorization of whatever came off the microscope picks our chunk shape instead of useful chunks for read/write performance (that 997 example). And those dimensions can change every experiment. The shapes that always divide are the ones I don't want, (1,1,1) or the whole array.
I'd like chunking and sharding to be invisible to our users and make "just work" across acquisition geometries, and I think this gets us closer. Thanks for flagging the PR!
Oh, also would this work well with rectilinear grids?
|
Thanks for sharing your perspectives. I want to state that I am still skeptical about the complexity that this would add to implementations that optimize around the current constraints in the sharding codec. I think we need more discussion in the ZFWG. |
|
it would definitely be good to see an impact assessment of this change for existing implementations. |
|
I can't speak to implementation cost, but I can offer a user perspective. @normanrz, one thing worth weighing: the case @srivarra described is a regular outer chunk grid, so keeping the strict requirement for regular grids and relaxing it only for Concretely, on a light-sheet reconstruction with
Round the shard up instead and part of it sits outside the array, so no write ever covers a full shard. Two measured consequences:
We've worked around it by deleting the shards a write owns before writing (czbiohub-sf/iohub#455), which is fine but is a workaround every writer would have to reinvent. To be fair about scope: that recoverability problem is fixable in implementations, by treating a write that covers a boundary shard's full in-bounds region as complete — no spec change needed, and zarr-developers/zarr-python#757 already did the equivalent for unsharded chunks. What that doesn't fix is the first problem: when an extent factors badly, the spec forbids the subchunk shape you actually want. That part needs #370. |
|
For recovering from failed partial writes, you should write to a temp filename and rename when complete. If you want your shard shape to exactly divide your overall shape you will still be quite constrained. Allowing dimensions to be marked non-resizable would be one possibly more elegant solution to avoiding extra overhead on boundary chunks. Still the overhead you mention --- the 126ms is avoided by using a temporary file, and how does the 85ms compare to the case when you don't need to check for an existing file? |
xref: zarr-developers/zarr-extensions#34