Repository navigation
fix(alias_table): walk parent groups iteratively and stop at the file root - #55
Merged
Merged
Conversation
… root convert_parents_to_structs recursed on h5group.parent, but in h5py the root group is its own parent, so a file whose root already carries a datatype attribute sent the walk into infinite recursion. Rewrite the walk as a loop that terminates at '/', never invents a datatype on the root, and stops early when the child is already a registered member. Add a test aliasing into a new top-level group of a file with an annotated root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 973d031)
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The iterative root-terminating walk addresses the documented recursion failure mode and is covered by a targeted regression test reproducing the production file shape.
Pull request overview
Fixes an HDF5 alias-table crash caused by convert_parents_to_structs recursing forever at the HDF5 root (in h5py the root group is its own parent), by switching to an iterative parent-walk with an explicit stop at / while preserving the prior “only update root if it already has datatype” behavior.
Changes:
- Rewrite
convert_parents_to_structsto walk parent groups iteratively and terminate reliably at the file root. - Harden parent handling by checking for the presence of the
datatypeattribute (avoidsKeyErrorwhen parents have other attrs). - Add regression test covering the real-world LH5 shape where the root already carries a
datatypestruct.
File summaries
| File | Description |
|---|---|
| tests/test_utils_helpers.py | Adds regression test asserting aliasing into a new top-level group correctly updates / exactly once and avoids empty members. |
| src/legenddataflowscripts/utils/alias_table.py | Replaces recursive parent traversal with an iterative loop that stops at /, preventing infinite recursion and root corruption. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
The bug
convert_parents_to_structsrecursed onh5group.parent— but in h5py the root group is its own parent, so the walk could only terminate via the branch requiring/to have zero attributes. Real LH5 files always carry adatatypeattribute on the root, so every alias write into a new top-level group (dsp/<detector>,hit/<detector>, …) recursed untilRecursionErrorafter ~1000 frames. At the root,name.split('/')[-1]is'', which the field filter strips, so the membership early-return never fired and each pass appended an empty field — and because the file is open in append mode those partial writes are flushed, leaving a crashed file's root datatype corrupted asstruct{...,dsp,,,…}.Every
build-tier-dsp/build-tier-hitjob dies at the "Creating alias table" step, after the full processing time is already spent. In thev3.5.0dev1production (which installs the released 0.4.0a1) this currently accounts for 1354 failed dsp jobs (p19 cal + fft).The fix
Rewrite the walk iteratively with an explicit stop at the root. Deliberately preserved: the root's
datatypeis still extended with new top-level members (LGDO readers need it); a root with nodatatypeis left untouched; the early return when the child is already a member; the non-struct-parentValueErrorand substring-safe field parsing. Incidental hardening: keying on'datatype' in attrsavoids aKeyErroron parents carrying only other attributes.Regression test included (
test_alias_table_annotated_root) reproducing the production file shape — a root group that already carriesdatatype— which is exactly the case the existing test fixture missed. Verified on real p18 data: the produced dsp file's root gainsdspexactly once with no empty fields, and all 135 detector aliases resolve as hard links readable through both paths.Cherry-picked from
integration-fable(973d031); 18/18 tests pass on current main.🤖 Generated with Claude Code