Batch Zarr attribute writes: fixes the split hang in CI - #11
Merged
Merged
Conversation
pytest-timeout caught the hang I could not locate before. The main thread was
stuck in zarr's sync-over-async bridge:
split_store -> subset_h5ad -> subset_axis_group -> copy_attrs
-> zarr Attributes.__setitem__ -> Group.update_attributes
-> zarr.core.sync.sync
Every `attrs[k] = v` on a Zarr group persists the whole metadata document
through that bridge, so copy_attrs was doing one round trip per attribute and
set_encoding two per element. `split` repeats all of it once per output group,
which is why only that command showed it; the thread dump had 288 threads and
four idle asyncio workers.
Attributes are now written in a single `put()` on Zarr, falling back to
assignment on HDF5, which has no batch API. That removes most of the round
trips rather than just making them less likely to collide.
The earlier Rich live-display fix stands on its own merits -- nesting two live
displays on one console is wrong regardless -- but it was not the cause.
Ten consecutive runs of the split and zarr tests: no hangs, no failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pytest-timeoutearned its place immediately: it caught the intermittent CI hang I couldn't locate before, on the very first run after it was added.Root cause
The main thread was stuck in zarr's sync-over-async bridge:
Every
attrs[k] = von a Zarr group persists the whole metadata document through that bridge.copy_attrswas doing one round trip per attribute, andset_encodingtwo per element.splitrepeats that once per output group, which is why only that command exhibited it — the thread dump showed 288 threads created and four idle asyncio workers.Fix
Attributes are written in a single
put()on Zarr, falling back to assignment on HDF5, which has no batch API. That removes most of the round trips rather than merely making a collision less likely.On the previous attempt
I earlier fixed
subset_h5adnesting a RichProgressinside aconsole.statuson the same console, and suggested that might be the cause. It wasn't — the hang recurred afterwards. That change stands on its own merits (Rich permits one live display per console, and the nesting was plainly wrong), but it should not be credited with fixing this.Evidence
🤖 Generated with Claude Code