Decode HDF5 text that its writer declared ASCII - #16
Merged
Conversation
hdmf keys its string-decoding decision directly off the character set. A writer that declares H5T_CSET_ASCII rather than UTF-8 -- aqnwb, and so every recording Orion produces -- therefore hands every string back as bytes where a pynwb-written file gives str (NeurodataWithoutBorders/aqnwb#316). Nothing raises. Three read boundaries in the slicer forwarded those bytes into places that only ever compare or parse them: - Interval tables were stringified with `map(str, ...)`, which on bytes yields the repr. A marker payload arrived as "b'{\"cause\": ...}'" -- text that json.loads rejects, so a consumer like intent's cursor marker parser dropped every marker and reported nothing wrong. - The electrodes `label` column becomes the ch-axis coordinates that every downstream name-based channel selection matches against. - `Device.manufacturer` is parsed as a prefix when a caller names a stream by bare device; a bytes value there would discard the stream silently. hdmf happens to decode ASCII attributes even though it leaves datasets alone, so that last one is a guard rather than a live bug. A text series (markers, annotations) is decoded once at load rather than at each slice, so the three places that index `dset` -- both slicer read paths and the iterator's -- can't each forget to. The suite could not have caught this: written by pynwb throughout, its text reads back as str and none of these paths is exercised. The new fixture restates the strings as fixed-length ASCII and asserts first that they really do read back as bytes; without that guard it would pass against the very bug it exists to catch. Fixing aqnwb upstream won't retire it either -- files already on disk keep their character set.
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.
Why
hdmf keys its string-decoding decision directly off the HDF5 character set. A writer that declares its strings
H5T_CSET_ASCIIrather than UTF-8 — aqnwb, and therefore every recording Orion produces — hands every string back asbyteswhere a pynwb-written file givesstr. Filed upstream as NeurodataWithoutBorders/aqnwb#316, and being looked at.That fix will not retire this one. Files already on disk keep the character set they were written with, so a reader has to survive both indefinitely.
Nothing raises.
str(value)onbytesyields the repr —"b'cond_0'"— which looks like text, compares equal to nothing, and parses as no JSON.What
Three read boundaries in
NWBSlicerforwarded those bytes into places that only ever compare or parse them:slicer.py:239) were stringified withmap(str, ...). A marker payload arrived as"b'{\"cause\": ...}'"— textjson.loadsrejects, so a consumer such as INTENT's cursor marker parser dropped every marker and reported nothing wrong.labelcolumn (slicer.py:344) becomes the ch-axis coordinates that every downstream name-based channel selection matches against.Device.manufacturer(slicer.py:116) is parsed as a prefix when a caller names a stream by bare device (stream_keys=["NPLAY"]matchingCereLink_NPLAY); a bytes value there would discard the stream in silence.The third is a guard, not a live bug: hdmf happens to decode ASCII attributes on the way out even though it leaves datasets alone. I only found that out because the test asserting otherwise failed, so the comment and a test now pin the asymmetry rather than claim a fix.
A text series (markers, annotations) is decoded once at load rather than at each slice, so the three places that index
dset— both slicer read paths and the iterator's — can't each forget to. Safe to materialize: text series are markers, short and few, on the order of the interval tables already read whole beside them.as_text/as_text_arrayare exported from the package, since a downstream reader of these files needs the same primitive.Test
The suite could not have caught this. Written by pynwb throughout, its text reads back as
strand none of these paths is exercised — which is how the bug reached hardware.The new
ascii_nwb_pathfixture restates the strings as fixed-length ASCII the way aqnwb writes them, carrying one of each kind of text a reader has to survive: spec-defined and custom electrodes columns, a custom interval-table column, a marker series, and the Devicemanufacturerattribute. It asserts first that they really do read back asbytes; without that guard the tests would pass against the very bug they exist to catch.Reverting
slicer.pyalone fails exactly the four data-path tests.Verification
ruff check/ruff formatclean.