feat(pose-file): revision-1 format core — writer, reader, validator, fixtures - #455
feat(pose-file): revision-1 format core — writer, reader, validator, fixtures#455bergsalex wants to merge 16 commits into
Conversation
| from jabs.io.internal.pose_file.reader import read_component, read_pose_file | ||
| from jabs.io.internal.pose_file.validate import validate | ||
|
|
||
| DATA = Path(__file__).parents[2] / "data" / "pose-format" |
There was a problem hiding this comment.
The entire conformance fixture corpus and its generator are excluded from the commit by .gitignore's bare data rule, so all 13 conformance tests fail on any fresh clone.
git ls-tree -r <head> --name-only | grep -c jabs-io/tests/data returns 0, and git check-ignore -v packages/jabs-io/tests/data/pose-format/valid-minimal.h5 reports .gitignore:21:data — the bare data pattern on line 21 matches any directory named data at any depth, swallowing all 8 .h5 fixtures and generate.py, so a clone cannot even regenerate them.
Proof of consequence: moving the untracked corpus aside and re-running the suite gives 13 failed, 63 passed — every test in this file, FileNotFoundError out of h5py.File. The suite passes only on a machine where the ignored files happen to exist on disk. This PR has a commit titled "test(pose-file): conformance fixture corpus" whose stat shows one file changed: test_conformance.py.
The ADR makes the corpus a shipped deliverable ("Conformance fixtures ship with the specification in packages/jabs-io/tests/data/pose-format/"), and its stated purpose is that "any independent implementation can test against" it — so today no independent implementation, and no reviewer, can.
Fix: negate the pattern, e.g. add !packages/jabs-io/tests/data/ (and confirm with git check-ignore -v).
| with h5py.File(path, "w") as h5: | ||
| h5.attrs["jabs_format"] = FORMAT_ID | ||
| h5.attrs["schema_revision"] = np.int32(SCHEMA_REVISION) | ||
| h5.create_dataset("manifest", data=json.dumps(manifest), dtype=string_dtype) |
There was a problem hiding this comment.
Both json.dumps calls sit INSIDE the h5py open, so an unserialisable provenance value — or a component id that is a path prefix of another — destroys the destination file after truncation.
The docstring promises "a validation failure must not destroy an existing file", but only the two schema validations happen before the open. json.dumps(manifest) and json.dumps(provenance) are on lines 85–86, inside with h5py.File(path, "w"), which has already truncated the destination.
Trigger 1 — ordinary caller input. ProvenanceRecord(parameters={"seed": np.int64(42)}) passes validate_provenance (the schema says only parameters: {type: object}, and jsonschema never descends into the values; np.int64 is not an int subclass — unlike np.float64, which is a float subclass and survives). Then json.dumps raises TypeError: Object of type int64 is not JSON serializable. Verified overwriting a valid 7-component file: 24176 → 6160 bytes, 7 components → 1, root keys ['manifest'], and read_pose_file on the remains raises KeyError for the missing /provenance.
Trigger 2 — prefix-colliding ids. jabs.pose.points (→ /jabs/pose/points, a Dataset) and jabs.pose.points.confidence (→ needs that name to be a Group) both pass Component.__post_init__, PoseFile.__post_init__ and validate_manifest, then create_dataset raises TypeError: Incompatible object (Dataset) already exists mid-write. Verified: a destination holding dataset years_of_work was left with ['jabs','manifest','provenance'] and the original gone; validate() then reports component_path_exists on the wreckage — a corrupt file that still identifies as a pose file.
And validate() crashes on the manifest-only shell trigger 1 leaves behind (see the /provenance comment), so the tool you would reach for to diagnose the damage fails too.
Pre-flight can only cover the failure modes someone thought of; ENOSPC, EIO and a signal between the open and the close are unpreventable at this altitude.
Deeper fix: serialise both documents before the open, and write to a temp file in the destination's own directory (os.replace is atomic only within one filesystem) then os.replace on success. Then no post-open error — anticipated or not — can touch the destination, and pre-flight validation becomes an early-error convenience rather than the load-bearing safety mechanism.
(The test that purports to cover this cannot fail — see the test_writer.py comment.)
| ) | ||
| with h5py.File(path, "r") as h5: | ||
| dataset = h5[spec["path"]] | ||
| return dataset[frames] if frames is not None else dataset[()] |
There was a problem hiding this comment.
read_component(frames=...) slices dimension 0 after only checking that "frame" appears somewhere in axes, so any component whose frame axis is not axis 0 silently returns the wrong data.
The guard on line 100 is a membership test — if frames is not None and "frame" not in spec["axes"] — and then line 107 does dataset[frames], which always windows axis 0. The axis position (spec["axes"].index("frame")) is never computed.
Verified on a file validate() accepts with zero findings: axes=("slot","frame"), shape (3,10), read_component(path, id, frames=slice(0,2)) returned shape (2,10) — two whole slot rows across all 10 frames — where the correct answer is full[:, 0:2], shape (3,2). No error, no warning, and the result even has a plausible shape.
Nothing constrains axis order: the schema's axes is {type: array, items: {type: string}}, and validate.py checks only arity. ADR 0002 states the opposite requirement explicitly:
Tooling that subsets a frame range operates on any component whose
axescontainsframe(slice the axis) orsample(…), regardless of namespace — which is what makes extensions survive clipping.
and "axes names each dimension of the array in order". So this breaks precisely the foreign-producer extensibility that windowing exists for, and silently.
Note validate.py:306 already does the lookup correctly for the sample axis (spec["axes"].index("sample")).
Fix:
axis = spec["axes"].index("frame")
index = (slice(None),) * axis + (frames,)
return dataset[index]
| if manifest is None: | ||
| return findings | ||
|
|
||
| provenance_records = set(json.loads(h5["provenance"][()]).get("records", {})) |
There was a problem hiding this comment.
validate() re-reads /provenance outside any guard, so a missing, malformed or wrong-typed provenance document raises out of the function instead of returning the Finding it already recorded.
_check_documents correctly catches the failure and appends a provenance_schema Finding — but its early return on line 110 gates only on manifest_schema, so it returns the manifest and this line dereferences h5["provenance"] a second time, unguarded.
Verified three modes, each after the correct Finding had already been recorded:
/provenance |
result |
|---|---|
| dataset deleted | KeyError: "Unable to synchronously open object (object 'provenance' doesn't exist)" |
"{not json" |
json.decoder.JSONDecodeError |
"[]" (valid JSON, wrong type) |
AttributeError: 'list' object has no attribute 'get' |
The documented contract is Returns: Findings, errors first. An empty list means the file conforms., and this is the entry point aimed at untrusted third-party files — so every caller and the conformance harness crashes rather than being told what is wrong. It is also exactly the wreckage the writer-truncation bug produces (a manifest-only shell), so the diagnostic tool fails on the file that most needs it.
There is no invalid-provenance*.h5 fixture, which is why this was not caught.
Fix: have _check_documents return the parsed provenance (or its record keys) alongside the manifest, and abort the pass on a provenance_schema finding the same way it aborts on manifest_schema. Also add Raises: to the docstring, or make the function total.
| ) | ||
| ) | ||
| elif "keypoint" in spec["axes"]: | ||
| axis = spec["shape"][spec["axes"].index("keypoint")] |
There was a problem hiding this comment.
After recording the non-fatal axes_arity finding the pass keeps going and indexes spec["shape"] by a position taken from spec["axes"], so the validator crashes on exactly the file that check exists to diagnose.
The manifest schema does not couple len(axes) to len(shape) — axes has minItems: 1, shape has no relation to it — so a manifest with axes: ["frame","slot","keypoint","coord"] and shape: [4] is schema-clean (verified: validate_manifest returned []).
validate() then appends the axes_arity ERROR, falls through to the skeleton branch, evaluates spec["shape"][spec["axes"].index("keypoint")] == spec["shape"][2], and raises IndexError: list index out of range — escaping validate() entirely, so the caller gets a traceback instead of the finding that was already in the list.
_check_sparse_index has the identical hazard at line 306 via spec["shape"][spec["axes"].index("sample")], reachable with axes: ["sample"] and shape: [].
Fix: return from _check_component immediately after appending axes_arity — every index-based lookup below it is unsound once the arity disagrees.
| Finding(ERROR, "sparse_index_valid", f"{index_id}: index must be one-dimensional") | ||
| ) | ||
| return | ||
| if values.size and not np.all(np.diff(values) > 0): |
There was a problem hiding this comment.
The strictly-increasing sparse-index check uses np.diff, which wraps on unsigned dtypes, so a decreasing uint32 index — the dtype the ADR prescribes for .frame_index — validates completely clean.
np.diff(np.array([5,3], dtype=np.uint32)) is 4294967294, which is > 0, so np.all(...) is True.
Verified end to end: a uint32 index [7,4,1] produced no sparse_index_valid finding, while the same values as int32 correctly errored:
uint32 [7,4,1] -> validate(): (no sparse_index_valid finding)
int32 [7,4,1] -> error sparse_index_valid: index must be strictly increasing
This is not a hypothetical dtype: ADR 0002 specifies uint32 for .frame_index and requires it strictly increasing as an error row, and the shipped valid-sparse.h5 fixture's own frame_index is uint32 (generate.py writes np.array([0,3,6], dtype=np.uint32)). So the rule is inert for every conformant file, and a consumer that bisects the index for a frame window silently reads the wrong samples. Duplicates (diff == 0) are still caught; only decreasing indices are blessed.
The adjacent values.min() < 0 on line 298 is likewise dead code for any unsigned dtype — it can never fire.
Fix: widen before comparing, e.g. values = np.asarray(...).astype(np.int64) ahead of the diff and the range check.
Rider: _check_sparse_index is invoked once per referencing component, so one shared index emits N identical findings (3 components sharing an index → 3 copies of the same message).
| "uint16", | ||
| "uint32", | ||
| "uint64", | ||
| "bool", |
There was a problem hiding this comment.
DTYPE_NAMES silently narrows the schema's dtype enum by omitting "string", so jabs.identity.external_ids — a component the ADR's own catalog defines — is unwritable, unreadable and falsely reported invalid.
schemas/manifest-1.json's $defs/dtype has 12 members ending in "string", and the ADR catalog defines:
jabs.identity.external_ids|/jabs/identity/external_ids| identity | I | string | optional display names
This frozenset lists 11, and no numpy dtype ever yields the name "string". All three legs fail (verified):
- Unwritable.
Component(id="jabs.identity.external_ids", axes=("identity",), data=np.array(["a","b"]), …)raisesValueError: unsupported dtype 'str256'—'bytes64'forS8,'object'for vlen. The reference writer cannot produce a component the specification mandates. - Falsely invalid. A hand-built schema-valid file (
validate_manifest→[]) with an h5py vlen-utf8 dataset makesvalidate.py:145emit a bogus ERROR:declares string(2,) but the dataset is object(2,)— a conformant file reported non-conformant. - Unreadable.
read_pose_fileon that same file raisesValueError: unsupported dtype 'object'.
The comment above the frozenset ("The dtypes the manifest schema admits") asserts the opposite of what the code does.
Fix, either direction — but pick one owner: a single dtype module owning the mapping in both directions (to_storage(name) with "string" → h5py.string_dtype(encoding="utf-8"), and from_dataset(dset) mapping object/vlen-str back to "string"), used by Component.dtype, the writer's create_dataset, the reader and validate; or drop "string" from the revision-1 schema enum and the ADR catalog (the format is additive-only, so a later revision can add it back with the mapping that makes it real).
Either way, add a test asserting DTYPE_NAMES == set(MANIFEST_SCHEMA["$defs"]["dtype"]["enum"]), so the schema and the code cannot drift again.
| "axes": list(component.axes), | ||
| "dtype": component.dtype, | ||
| "shape": [int(n) for n in component.data.shape], | ||
| "encoding": {"kind": "dense"}, |
There was a problem hiding this comment.
encoding is hardcoded to dense here and never read back by the reader, so a schema-valid ragged/rle file is silently relabeled dense on rewrite and its offsets dataset dropped — with zero findings before or after.
Component has no encoding field and grep -n encoding reader.py finds nothing, so read_pose_file discards the declaration; _component_entry then re-emits {"kind": "dense"} unconditionally.
Verified: an rle component (validate() → []) round-tripped to encoding {'kind': 'dense'}, its /jabs/segmentation/offsets absent from the output, and validate(rewritten) → [] again. The encoding is unrecoverable while the bytes are still run values, and read_component hands the undecoded RLE buffer to callers as if it were a dense array.
Separately, validate.py contains no reference to group_offsets, instance_offsets or "ragged" at all, though the ADR has two error rows for offset integrity:
ragged
group_offsetsis non-decreasing, starts at 0, ends atshape[0]of the payload, lengthnum_groups + 1| error
Verified: a ragged file with group_offsets [5, 3, 0] (decreasing, wrong start, wrong end, wrong length) and 2-element instance_offsets where 25 are required returns no findings.
The __init__.py docstring scoping this increment to dense is not the issue — a deliberate not-implemented refuses loudly. This accepts and corrupts what it cannot decode, which contradicts the reader's own premise ("a reader asks what a file contains") and the ADR's "jabs-io implements all defined encodings and normalizes them on read".
Minimum fix for this increment: refuse a non-dense encoding on read with a clear error, and carry encoding on Component so it round-trips verbatim rather than being re-asserted.
| VideoInfo, | ||
| ) | ||
| from jabs.io.internal.pose_file.validate import Finding, validate | ||
| from jabs.io.internal.pose_file.writer import write_pose_file |
There was a problem hiding this comment.
PoseFile is never registered with the jabs.io adapter registry, so jabs.io.save(pose_file, path) — the package's entire public write API — misroutes to DataclassHDF5Adapter, raises, and destroys the destination file.
Verified end to end:
jabs.io public API: ['load', 'save']
get_adapter(StorageFormat.HDF5, PoseFile) -> DataclassHDF5Adapter
write_pose_file(pf, victim) # valid file: ['jabs','manifest','provenance'], 10328 bytes
jabs.io.save(pf, victim) # TypeError: Object dtype dtype('O') has no native HDF5 equivalent
destination after save(): 6448 bytes
root keys now: ['video'] root attrs: ['dimensions','skeletons']
manifest still present: False
Nothing registers an adapter for PoseFile, and DataclassHDF5Adapter is registered polymorphically at priority 5 with can_handle = is_dataclass, so it claims PoseFile by default. HDF5Adapter.write opens h5py.File(path, "w") before writing anything, so this is a second, independent path to the same data destruction as the json.dumps-inside-the-open bug — and this one goes through the front door.
Consequences: jabs.io.__init__ exports only load and save, so the public API cannot read or write the new format at all; load(path, PoseData) on a revision-1 file lands in PoseHDF5Adapter.read and raises NotImplementedError telling the user to import an internal path by hand. This PR's commit is titled "public API surface and adapter seam", and the seam delivered is a legacy=None sentinel on the legacy adapter — whose can_handle is data_type is PoseData, so it can never route a PoseFile read regardless of what the mapping does.
Fix, following the precedent already in this package (internal/prediction/hdf5.py:35, which documents exactly this shape — "Overrides write and read directly rather than using _write_one/_read_one"):
@register_adapter(StorageFormat.HDF5, PoseFile, priority=10)
class PoseFileHDF5Adapter(HDF5Adapter):
def write(self, data, path, **kw): write_pose_file(data, path)
def read(self, path, data_type=None, **kw): return read_pose_file(path)Override write/read directly — not _write_one, whose base write() truncates before validation.
| entry["description"] = skeleton.description | ||
| skeletons[skeleton_id] = entry | ||
|
|
||
| manifest: dict = { |
There was a problem hiding this comment.
A read→write round trip silently drops /attachments, the manifest's attachments array and every extra object, and relocates any payload not stored at the id-derived path — which the ADR names a specification violation.
PoseFile has no attachments field, Component has no extra field, and this manifest literal emits only format/schema_revision/created/dimensions/video/components (+skeletons). write_pose_file writes only /manifest, /provenance and component payloads, so attachment datasets are not copied either.
Verified on a file that validates with zero findings, after read_pose_file → write_pose_file:
attachments kept? False
manifest `attachments` ABSENT
root `extra` kept? False
components[0]['extra']? ABSENT
datasets: ['jabs','manifest','org.jax.gait','provenance']
path: /somewhere/else/strides -> /org.jax.gait/stride_length
validate(rewritten): [] # nothing reports any of it
The ADR is explicit:
A tool that copies or transforms a file must carry attachments through verbatim. … Silently dropping an attachment is a specification violation. Dropping is irrecoverable; preserving with a recorded caveat is not.
and on extra:
extraexists on the manifest root and on every component … It is the manifest's own extension point, and it is whyadditionalPropertiescan safely befalseeverywhere else.
So dropping extra disables the format's only forward-compatibility mechanism. And because Component.path is a derived property (types.py:177) while the reader reads from spec["path"], a payload legitimately stored elsewhere (the schema types path as any ^/[^\0]*$) is silently moved on rewrite — breaking any sibling file, external index or byte-range consumer that recorded the old path.
This is the documented copy/transform path (the ADR's replacement for clip_utils.py), and PoseFile's docstring — "The full contents of one pose file" — is false as written.
Fix: give Component extra and a stored path (with the current derivation as its default), give PoseFile an attachments field, carry attachment datasets through on write, and add a validate() check that a jabs.* component's path equals the canonical derivation — so the convention is asserted where files are certified rather than assumed where they are written.
| ) | ||
| ) | ||
|
|
||
| skeleton_id = spec.get("skeleton") |
There was a problem hiding this comment.
The skeleton_reference check implements only the first half of its ADR error row — references resolve — and never bounds-checks edge indices, so validate() certifies a file the library's own reader refuses to open.
The ADR row is one row with two clauses:
skeletonreferences resolve; every edge index< len(body_parts)| error
This block checks resolution and keypoint_axis_length; skeleton["edges"] is never inspected anywhere in validate.py.
Verified: appending edge [0, 500] to a 12-body-part skeleton makes validate() return no findings, and read_pose_file on that same file then raises:
ValueError: skeleton edge out of range for 12 body parts: [(0, 500)]
from Skeleton.__post_init__. So the validator blesses a file the reference reader cannot open — and the bound is enforced only for files this library wrote, not for the third-party producers the namespace rule exists to admit. A consumer that renders edges without its own bounds check gets an IndexError into the keypoint array instead.
The schema cannot express the constraint either ($defs/skeleton.edges items are only {type: integer, minimum: 0} — no upper bound), so validate.py is the only possible home for it.
Note the existing test cannot catch this: test_keypoint_axis_must_match_the_skeleton mutates body_parts and edges together, so it never probes the edge rule, and skeleton_reference has no test at all.
This is a symptom of a broader split worth addressing: types.py.__post_init__, the JSON Schema, and validate.py each enforce an overlapping but different rule set, with no single owner and no test asserting they agree. identity_le_slot and the duplicate-id detection are character-identical copies across types.py and validate.py, while this rule landed in only one layer.
| ) | ||
| ) | ||
|
|
||
| reference = spec["missing"].get("mask") or spec["missing"].get("length") |
There was a problem hiding this comment.
Three validation-table clauses are implemented strictly weaker than their spec text: the mask/length shape-compatibility clause, the sparse-index sample-axis clause, and the frame-axis-length invariant are all absent.
(a) mask/length shape. ADR: "a mask or length reference resolves to an existing component with a compatible shape | error". This line tests only reference not in declared_ids. Verified: a (8,3,2,2) component naming a (2,4) component as its mask — shapes that cannot broadcast — produces no finding, so an undecodable file passes.
(b) sparse index axis. ADR: "sparse.index resolves to a 1-D sample-axis component, strictly increasing, within [0, video.frame_count) | error". _check_sparse_index checks ndim, monotonicity, range and length but never inspects index_spec["axes"]. Verified: rewriting the index component's axes to ["frame"] and dropping its sparse key was accepted with 0 findings — precisely the frame-vs-sample confusion the ADR says the two axis names exist to prevent.
(c) frame axis length. ADR Axes table: "an axis named frame always has length dimensions.frame". Checked nowhere. Verified: dimensions {frame: 100000, slot: 7, identity: 3} against a (2,1) frame×slot payload validated with zero findings and read cleanly. dimensions.slot/dimensions.identity are likewise never compared to any axis, and dimensions.frame is never reconciled with video.frame_count even though _check_sparse_index bounds the sparse index by the latter.
So dimensions — a required manifest key — is decorative. A consumer that allocates or indexes from dimensions.frame or video.frame_count reads out of range or truncates, with the file certified conformant.
The ADR names this exact risk as the reason validate() exists:
The manifest is a second source of truth about shapes and dtypes and can disagree with the arrays.
validate()exists because of this.
Also worth noting: the or in spec["missing"].get("mask") or spec["missing"].get("length") (mirrored at types.py:311) treats a falsy id as absent, so an empty-string mask silently falls through to the length lookup.
None of the five sparse_* checks, nor coord_declarations, skeleton_reference, component_id_unique, namespace_well_formed, rle_needs_dimensions or provenance_schema, has any test — 10 of 21 check= rules are unexercised, so any of them could be deleted or inverted with the suite still green.
| assert entry["layout"]["compression"] == (dataset.compression or "none") | ||
|
|
||
|
|
||
| def test_invalid_pose_file_does_not_touch_disk(tmp_path, sample_pose_file): |
There was a problem hiding this comment.
This test asserts the writer's most safety-critical property while never calling the writer, so it cannot fail — and the property it claims to cover is in fact broken.
The docstring states the invariant under test ("h5py truncates at open, so validation must happen before opening"), but the body only invokes the PoseFile constructor inside pytest.raises. The ValueError comes from PoseFile.__post_init__ (types.py:282), not from write_pose_file, which appears nowhere in this test. Nothing ever opens path, so
assert path.read_bytes() == b"sentinel"is a tautology: the sentinel survives no matter what the writer does. The unused sample_pose_file parameter is a symptom of the same omission.
Consequence: moving validate_manifest/validate_provenance to after h5py.File(path, "w") would leave all 76 tests green — and the real post-open truncation hazard did ship (see the writer.py comment: json.dumps runs inside the open, and a np.int64 in ProvenanceRecord.parameters or a prefix-colliding component id destroys the destination).
To exercise the real path the test needs a PoseFile that constructs successfully but whose built manifest fails schema validation, then asserts the sentinel survives. A convenient one exists: a skeleton id that is not a dotted componentId (skeletons={"mouse": ...}) constructs fine — PoseFile.__post_init__ checks only that the key exists, never its format — and is rejected by the schema's propertyNames at write time.
While here, two adjacent coverage gaps in the same area: 9 of the 17 raise ValueError sites in types.py have no test (duplicate component ids, unknown skeleton/provenance/sparse-index references, dangling mask, missing frame/slot dimension key, empty body_parts, unsupported dtype, and the negative half of the edge-range check), and those cross-reference rules are the only thing standing between a caller and a corrupt file, since four of them have no equivalent enforcement anywhere else in the write path.
| NotAPoseFileError: If the file is not a ``jabs.pose-file``. | ||
| """ | ||
| with h5py.File(path, "r") as h5: | ||
| if h5.attrs.get("jabs_format") != FORMAT_ID: |
There was a problem hiding this comment.
Three unguarded assumptions about how h5py represents foreign HDF5 metadata: one rejects genuine pose files, two crash validate() instead of reporting a Finding.
(a) jabs_format compared with != against a str (this line, mirrored at validate.py:56). h5py returns np.bytes_ for an attribute written as fixed-length ASCII — the default for the C, Fortran, MATLAB (h5writeatt) and Julia HDF5 APIs — and np.bytes_(b'jabs.pose-file') != 'jabs.pose-file' is True. Verified:
attrs['jabs_format'] -> np.bytes_(b'jabs.pose-file') <class 'numpy.bytes_'>
read_manifest raised: NotAPoseFileError: ... is not a JABS pose file, not a jabs.pose-file
validate() -> [('root_attrs', "jabs_format is np.bytes_(b'jabs.pose-file'), expected 'jabs.pose-file'")]
Files this writer produces are unaffected (it writes vlen utf-8) — the breakage is exactly the cross-implementation interop the published schema and the conformance corpus exist to enable. Note the message degenerates too, because _describe_other_format finds no poseest group.
(b) int(h5.attrs["schema_revision"]) at validate.py:68 is unguarded. Verified: np.array([1], dtype=np.int32) raises TypeError: only 0-dimensional arrays can be converted to Python scalars; "1.0" raises ValueError. A shape-(1,) integer version attribute is not hypothetical — it is what legacy JABS writes for poseest/version, and this PR's own tests use np.array([6, 0], dtype=np.uint16). reader.py:39 defensively ravel()s the same shape; _check_root does not.
(c) if "attachments" in h5 at validate.py:369 is a name-existence test, not a group test. A producer that parks a single opaque blob at /attachments rather than under it makes h5["attachments"].visititems(...) raise AttributeError: 'Dataset' object has no attribute 'visititems' — verified — and this is precisely the opaque-payload area the ADR expects foreign tools to write.
_describe_other_format has two more of the same shape: a byte-string poseest/version raises ValueError, an empty version array raises IndexError, and a dangling soft link named poseest makes "poseest" in h5 true while h5.get("poseest", {}) returns the {} default, so {}.attrs raises AttributeError — all while building the message for the documented NotAPoseFileError.
Fixes: normalise the attribute before comparing (v.decode() if isinstance(v, bytes) else str(v)); wrap (b) in try/except (TypeError, ValueError) → Finding(ERROR, "root_attrs", …) or normalise via np.asarray(...).ravel()[0]; guard (c) with isinstance(h5["attachments"], h5py.Group).
| NotAPoseFileError: If the file is not a ``jabs.pose-file``. | ||
| """ | ||
| manifest = read_manifest(path) | ||
| parsed = parse_manifest(manifest) |
There was a problem hiding this comment.
No read path ever schema-validates the manifest, and read_pose_file enforces only the subset of rules PoseFile.__post_init__ happens to implement — so on this PR's own invalid corpus it silently returns wrong data for two fixtures and leaks three different exception types for the others.
grep -n validate_manifest reader.py → no match. read_manifest is just json.loads, and parse_manifest/parse_provenance both document "Args: manifest: A validated manifest document" — a precondition nothing in the read path ever establishes.
Run read_pose_file over the shipped invalid fixtures:
| fixture | validate() says |
read_pose_file does |
|---|---|---|
invalid-shape-mismatch.h5 |
dtype_shape_match error |
OK, silently — Component re-derives shape from the array, absorbing the manifest's lie |
invalid-keypoint-axis.h5 |
keypoint_axis_length error |
OK, silently — returns body_parts with 2 names against a 12-wide keypoint axis |
invalid-dangling-mask.h5 |
mask_reference error |
ValueError from a dataclass constructor |
invalid-missing-payload.h5 |
component_path_exists error |
raw h5py KeyError: "...object 'confidence' doesn't exist" |
The invalid-keypoint-axis row is the dangerous one: any consumer doing body_parts[k] now mislabels or IndexErrors on real coordinate data, with no error anywhere. And a malformed manifest surfaces as KeyError: 'dimensions' rather than a typed error.
Four behaviours for four invalid files, none of which the docstring lists (Raises: NotAPoseFileError). The three-layer split is the root cause: the JSON Schema, types.__post_init__ and validate.py each enforce an overlapping but different rule set, and the read path runs only the middle one.
Related, same mechanism — nothing checks that a declared path resolves to a Dataset rather than a Group. "path": "/jabs/pose" is schema-valid (hdf5Path is just ^/[^\0]*$) and exists as a group in every real pose file, so spec["path"] not in h5 passes and dataset.shape at validate.py:145 raises AttributeError: 'Group' object has no attribute 'shape' out of validate(). If /manifest itself is a group, h5["manifest"][()] raises TypeError, which the except (KeyError, ValueError) at validate.py:94 does not catch — and reader.py:59 has the identical hole.
Fix: call validate_manifest in read_manifest (or a strict=True mode) so the documented precondition holds, and isinstance(..., h5py.Dataset) before touching .shape/.dtype/.chunks.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate correctness issues affect reading, writing, schema enforcement, and validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces the revision-1 self-describing pose-file foundation in jabs-io.
Changes:
- Adds schemas, domain types, manifest handling, dense HDF5 I/O, and validation.
- Adds conformance fixtures and comprehensive tests.
- Preserves legacy V2 behavior while reserving revision-1 integration.
File summaries
| File | Description |
|---|---|
uv.lock |
Updates locked dependencies. |
packages/jabs-io/tests/internal/pose_file/test_writer.py |
Tests pose-file writing. |
packages/jabs-io/tests/internal/pose_file/test_validate.py |
Tests validation behavior. |
packages/jabs-io/tests/internal/pose_file/test_types.py |
Tests domain types and constraints. |
packages/jabs-io/tests/internal/pose_file/test_schema.py |
Tests schema validation. |
packages/jabs-io/tests/internal/pose_file/test_reader.py |
Tests reading and frame windowing. |
packages/jabs-io/tests/internal/pose_file/test_public_api.py |
Tests exports and legacy compatibility. |
packages/jabs-io/tests/internal/pose_file/test_manifest.py |
Tests manifest conversion. |
packages/jabs-io/tests/internal/pose_file/test_conformance.py |
Tests the conformance corpus. |
packages/jabs-io/tests/internal/pose_file/conftest.py |
Provides shared test fixtures. |
packages/jabs-io/tests/internal/pose_file/__init__.py |
Initializes the test package. |
packages/jabs-io/tests/data/pose-format/generate.py |
Generates conformance fixtures. |
packages/jabs-io/src/jabs/io/internal/pose/hdf5.py |
Adds revision-1 selection handling. |
packages/jabs-io/src/jabs/io/internal/pose_file/writer.py |
Writes dense revision-1 files. |
packages/jabs-io/src/jabs/io/internal/pose_file/validate.py |
Implements conformance validation. |
packages/jabs-io/src/jabs/io/internal/pose_file/types.py |
Defines pose-file domain types. |
packages/jabs-io/src/jabs/io/internal/pose_file/schemas/provenance-1.json |
Defines the provenance schema. |
packages/jabs-io/src/jabs/io/internal/pose_file/schemas/manifest-1.json |
Defines the manifest schema. |
packages/jabs-io/src/jabs/io/internal/pose_file/schema.py |
Loads and applies schemas. |
packages/jabs-io/src/jabs/io/internal/pose_file/reader.py |
Reads manifests and components. |
packages/jabs-io/src/jabs/io/internal/pose_file/manifest.py |
Builds and parses metadata documents. |
packages/jabs-io/src/jabs/io/internal/pose_file/__init__.py |
Exposes the pose-file API. |
packages/jabs-io/pyproject.toml |
Adds schema-validation dependencies. |
.gitignore |
Allows committed conformance data. |
Review details
Suppressed comments (3)
packages/jabs-io/src/jabs/io/internal/pose_file/reader.py:130
read_pose_filediscardsencoding.kindand constructs the same in-memoryComponentfor dense, ragged, and RLE declarations. A schema-valid optional encoding is therefore silently exposed as if its encoded payload were dense. Since this increment intentionally supports dense only, reject non-dense components explicitly before constructing them so callers cannot consume misrepresented data.
components = tuple(
Component(
id=spec["id"],
axes=tuple(spec["axes"]),
data=h5[spec["path"]][()],
packages/jabs-io/src/jabs/io/internal/pose_file/types.py:137
- The new public domain model uses bare
np.ndarrayand unparameterizeddictannotations, so consumers and static tooling cannot determine payload or declaration value types. Repository guidance requiresnumpy.typing.NDArray[...]and concrete modern collection types on all new APIs; please introduce aliases for these heterogeneous schema structures and type the array explicitly.
id: str
axes: tuple[str, ...]
data: np.ndarray
missing: dict
units: str | None = None
packages/jabs-io/tests/data/pose-format/generate.py:332
- This generator uses
print()for operational output, contrary to the repository's logging standard. Define a module logger and emit these fixture statistics with lazylogger.infoformatting instead.
for fixture in sorted(HERE.glob("*.h5")):
print(f"{fixture.name:34s} {fixture.stat().st_size / 1024:7.1f} KiB")
- Files reviewed: 22/32 changed files
- Comments generated: 14
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8c0c2bc to
acbb967
Compare
|
Read this as the author of the consumer named in ADR 0002 Requirement 2 — the in-browser pose overlay in JABS Hub, which reads windows over a Range proxy with h5wasm and will have to reimplement this reader in TypeScript. Comments are from that angle only; I have not run the suite. The design holds up well for a networked reader
One hazard the port will hit: manifest reads are per-call
This is not a bug in the Python — it is an API shape that makes the expensive thing invisible. Two options, either fine by me:
I mention it here rather than filing it later because Requirement 2 is a stated requirement of the format, and this is the one place the reference implementation and that requirement pull apart. Two smaller port notes
One thing I checked and liked
|
There was a problem hiding this comment.
🟡 Changes recommended
Multiple unresolved moderate correctness and robustness issues affect reading, writing, validation, metadata fidelity, and API option handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/jabs-io/src/jabs/io/internal/pose_file/validate.py:575
- Offsets are specified as
uint64, but this accepts any dtype and blindly casts it. Floating-point offsets can be truncated into apparently valid indexes, and nonnumeric strings raiseValueError, violatingvalidate()'s report-only contract. Reject non-uint64datasets before conversion.
values = np.asarray(node[()])
if values.ndim != 1:
return None
return values.astype(np.int64, copy=False)
packages/jabs-io/src/jabs/io/internal/pose_file/writer.py:79
- Chunking always limits axis 0, even though components may declare
frameon another axis (a case supported and tested by the reader). For a(slot, frame)component this can put the entire frame axis in every chunk, so reading one frame decompresses all frames. Select the axis namedframewhen present.
shape = component.data.shape
if not shape or any(dimension == 0 for dimension in shape):
return None
leading = min(_FRAME_CHUNK, shape[0])
return (leading, *shape[1:])
- Files reviewed: 30/40 changed files
- Comments generated: 15
- Review effort level: Balanced
| records = { | ||
| key: ProvenanceRecord( | ||
| producer=entry["producer"], | ||
| version=entry["version"], | ||
| created=entry["created"], |
There was a problem hiding this comment.
Correct, and it contradicted the lossless round-trip contract I had written into the reader's own docstring.
Fixed in 8fa7038: ProvenanceRecord, HistoryEntry and Provenance all carry extra, and both the build and parse paths preserve it. A test round-trips namespaced extras at all three levels.
| flat = np.atleast_1d(np.asarray(value)).ravel() | ||
| if flat.size == 0: | ||
| return None | ||
| return attr_text(flat[0].item() if hasattr(flat[0], "item") else flat[0]) |
There was a problem hiding this comment.
Confirmed by reproduction — attr_text(1) raised RecursionError with the limit lowered to 200.
Fixed in 8fa7038: exactly one unwrapping step, then a non-string scalar returns None. read_pose_file raises PoseFileError and validate() reports root_attrs, which is what a malformed attribute should produce.
| if frames is None: | ||
| return _payload(dataset, spec) | ||
| axis = spec["axes"].index("frame") | ||
| selector = (slice(None),) * axis + (frames,) | ||
| return dataset[selector] |
There was a problem hiding this comment.
Correct — the representation depended on whether frames was supplied. Fixed in 8fa7038: asstr() is applied to the sliced selection too, with a test asserting window == whole[1:3] for a string component.
| try: | ||
| provenance_raw = json.loads(h5["provenance"][()]) | ||
| except (KeyError, TypeError, ValueError) as error: | ||
| raise PoseFileError( | ||
| f"{path}: /provenance is missing or unreadable: {error}" | ||
| ) from error | ||
| provenance = parse_provenance(provenance_raw) |
There was a problem hiding this comment.
Correct. parse_provenance indexes required fields, so a schema-valid-looking object with an empty record leaked a KeyError past the documented PoseFileError contract.
Fixed in 8fa7038: read_pose_file validates the provenance document against PROVENANCE_SCHEMA before parsing, and translates failures to PoseFileError. It also rejects a non-object document explicitly.
| findings: list[Finding] = [] | ||
| with h5py.File(path, "r") as h5: |
There was a problem hiding this comment.
Correct, and it contradicted the contract in that function's own docstring. Fixed in 8fa7038: the open is wrapped, and a truncated or non-HDF5 file returns a single file_readable finding. Verified against a file containing the bytes not hdf5 at all.
| if manifest is None: | ||
| return findings | ||
|
|
||
| provenance_records = set(provenance.get("records", {})) |
There was a problem hiding this comment.
Correct. Fixed in 8fa7038: records falls back to an empty set unless it is a dictionary, so the schema finding is reported and the reference checks degrade instead of raising TypeError.
| h5.create_dataset( | ||
| component.path, | ||
| data=[str(value) for value in component.data.tolist()], | ||
| dtype=h5py.string_dtype(encoding="utf-8"), | ||
| ) |
There was a problem hiding this comment.
Correct — and it meant our own output could trip our own layout_matches_file warning. Fixed in 8fa7038: the string branch sets only the dtype and then goes through the same contiguous/chunked selection as numeric components, so one code path decides layout for everything.
| if component.dtype == "string": | ||
| # Variable-length UTF-8, so a reader on any HDF5 implementation gets | ||
| # text rather than this machine's fixed-width padding. | ||
| h5.create_dataset( | ||
| component.path, | ||
| data=[str(value) for value in component.data.tolist()], | ||
| dtype=h5py.string_dtype(encoding="utf-8"), | ||
| ) |
There was a problem hiding this comment.
Correct, and worse than described when I reproduced it: a (2,2) object array wrote as shape (2,) with the element b"[a, b]" — a file validate() then rejected for dtype_shape_match.
Fixed in 8fa7038: conversion is elementwise over ravel() with reshape back to the original shape, and bytes are decoded rather than repr'd, so b"a" round-trips as "a". Two tests: a 2-D string component round-tripping shape and contents, and byte strings coming back as text.
| layouts = {component.id: _layout_for(component) for component in pose_file.components} | ||
| manifest = build_manifest(pose_file, layouts=layouts, created=created) | ||
| provenance = build_provenance(pose_file.provenance) | ||
| errors = validate_manifest(manifest) + validate_provenance(provenance) |
There was a problem hiding this comment.
Agreed, and this is the finding I most wanted — it names the class rather than an instance, and it caught two of my own tests doing exactly what you describe.
Fixed in 8fa7038: write_pose_file runs validate() on the completed temporary file and refuses to os.replace if there are any errors. Since the write is already atomic, a refusal leaves the destination untouched. It is cheap — validate() reads indexes and offsets, never the payloads — with the one exception of the new RLE coverage rule, which is recorded as an open question on the ADR for that reason.
The two tests that had been writing invalid files now corrupt them after writing, which is the only way to build such a fixture. I also pushed the schema conditionals down into the domain constructors, so most of these are refused before a file is opened at all rather than after it is written.
acbb967 to
8fa7038
Compare
First increment of ADR 0002. The two JSON Schemas are extracted verbatim from the ADR rather than retyped, so the specification and the shipped schemas cannot drift, and they load from package data so a reader never depends on the repository layout. Tests cover the eight things the schema must reject -- a manifest with no video, a component with no missing policy, a coord component without coord_order, a sample axis without sparse and sparse without a sample axis, ragged without group_offsets, and ids that are a single segment or uppercase -- plus that a convert history entry cannot validate without saying where it came from and what it invented. Verified the schemas ship in the built wheel; uv_build includes them with no package-data configuration. jsonschema was already a root and jabs-behavior dependency, so declaring it on jabs-io pins to the same range rather than adding a new one. The uv.lock diff also picks up twelve greenlet platform wheels published upstream since the lock was last refreshed -- incidental to this change, but stripping them by hand would be worse than recording them here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Types that mirror the file rather than the science: a PoseFile is a set of Components, each carrying its array beside the declarations the manifest records. PoseData stays the science-facing view; mapping the two belongs with the JABS integration. Validation lives in __post_init__ so an invalid file cannot be built in memory, let alone written -- which is what lets the writer validate before opening the file, since h5py truncates at open time. Enforced here: ids are lowercase and at least two segments, a non-jabs namespace needs a reverse-DNS root of at least two segments, axes name every dimension, a coord axis requires units and coord_order, the sample/sparse_index pairing holds in both directions, dtypes are ones the schema admits, skeleton edges are in range, identity <= slot, component ids are unique, and every skeleton, provenance, sparse_index and mask reference resolves. Skeleton edges are pairs, not polylines: drawing an edge only when both endpoints are valid gives the same picture as splitting a polyline at missing keypoints, so gen_line_fragments has no equivalent here. Component sets eq=False because dataclass equality over a numpy payload returns an array rather than a bool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_manifest, build_provenance and their parsing counterparts, plus the shared fixtures the writer, reader and validator tests all build on. Two conventions the tests pin down. Optional component declarations are omitted rather than nulled, because the schema forbids nulls there. Unknown video fields are emitted as explicit null, because "we could not determine the frame dimensions" is a fact a converted file must be able to state -- clip_of is the exception, since the schema types it as an object. build_manifest takes the layouts the writer will actually apply, so the manifest reports what is on disk rather than an intention. That is the field the review round added after pointing out the prose claimed the manifest recorded layout when nothing did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Root attributes, the two JSON documents, and one dataset per component at its declared path. Keypoint-scale components are written contiguous and uncompressed. That is the ADR's storage policy and the reason is specific: contiguous storage is the only layout under which a frame range really is one byte range, since HDF5 chunks need not be adjacent or ordered on disk. A test asserts chunks is None and compression is None rather than trusting the comment. The writer computes the layout it will apply and passes it to build_manifest, so a further test can assert the declared layout matches the dataset on disk in both directions. Validation happens before h5py.File opens the destination, because "w" truncates at open time; a test writes a sentinel file and asserts an invalid input leaves it untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three entry points in increasing cost: read_manifest learns the contents without touching a payload, read_component reads one component and optionally only a window of its frame axis, read_pose_file loads everything. The windowed read is the access pattern hard requirement 2 exists for, and a test asserts the window equals the corresponding slice of the full read. Asking for a window of a component with no frame axis is an error naming the axis, rather than a silently wrong slice. Identification is the jabs_format attribute; nothing inspects schema_revision. A file that is not one gets an error naming what it actually is -- "a legacy pose_est_v6 file" when poseest/version says so, which is the message a user hitting this mid-migration needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements the ADR's validation table as Findings carrying a stable check name, so callers and tests assert on a rule rather than on message text. Two design points. A manifest schema failure stops the pass, because every structural check below it assumes a well-formed manifest and would otherwise produce cascading noise. And a newer schema_revision is a warning, never a refusal -- the format is additive-only, so a newer file is readable; recording the difference is provenance, not a branch. Errors: root attributes, both documents against their schemas, payload presence, declared dtype and shape against the dataset, axis arity, mask and length references, the sample/sparse pairing in both directions, the sparse index (one-dimensional, strictly increasing, in range, and the same length as the sample axis), provenance references, coord declarations, skeleton references, keypoint axis against the skeleton's body parts, identity <= slot, id uniqueness, namespace form, and RLE without video dimensions. Warnings: unknown video dimensions, declared layout disagreeing with the file, and an attachment the manifest never declared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight committed fixtures plus the generator that writes them: three valid (minimal, full, sparse) and five invalid, one per rule they break. Invalid fixtures are produced by writing a valid file and corrupting it, since the writer will not produce a file it would refuse. valid-full carries a foreign org.example.lab.whisker_angle component, and a test windows it by frame range without knowing anything about it -- the extensibility claim made executable rather than asserted in prose. It also carries an undeclared attachment, so the warning path has coverage. valid-sparse exposes a specification gap, noted for the ADR: the schema requires a sample axis and a sparse declaration to accompany each other in both directions, so a frame-index component -- which itself has a sample axis -- must name itself as its own index. The ADR's dynamic-objects table does not show that. Either the table gains the self-reference or the rule needs relaxing for index components; the fixture uses self-reference for now because that is what the shipped schema demands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One coherent surface from jabs.io.internal.pose_file: write_pose_file, read_pose_file, read_component, read_manifest, validate, and the types. PoseHDF5Adapter.write gains legacy=None to select the revision-1 format, which raises NotImplementedError naming what is missing -- the PoseData <-> PoseFile mapping, since PoseData is identity-major with no slots and no provenance. Doing that mapping badly here would be worse than not doing it, so it lands with the JABS integration. The existing read stub's message is extended to name the revision-1 reader alongside the legacy one, rather than adding a second read method. No existing behavior changes: legacy V2 write is unchanged and still tested, an unsupported legacy version is still a ValueError, and read was already NotImplementedError before this branch. Verified: root suite 1059 passed, jabs-io 375, jabs-core 88, jabs-behavior 113, ruff clean. jabs-vision could not be verified in this worktree -- neither torch nor the jabs.vision package installs under `uv sync --all-extras` -- and this branch touches nothing in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The commit titled "conformance fixture corpus" contained only the test module. .gitignore has a bare `data` rule, which matches any directory of that name at any depth, so `git add packages/jabs-io` silently added none of the eight fixtures and not the generator either -- leaving a suite that passed locally on untracked files and failed 13 tests on a fresh clone. Negates the rule for packages/jabs-io/tests/data/ and commits all nine files. Verified by running the conformance suite in a worktree containing only tracked files, which is the check that would have caught this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three verified data-destruction paths, all the same root cause: h5py truncates at open, so anything that fails after it costs the caller their existing file. - Both json.dumps calls sat inside the open. A numpy integer scalar in provenance parameters passes the schema (typed as a bare object, and np.int64 is not an int subclass) and fails serialization. Measured on a valid seven-component file: 24176 -> 6160 bytes, one component left. - jabs.io.save(), the package's actual public write API, resolved PoseFile to the polymorphic DataclassHDF5Adapter -- PoseFile is a dataclass, so it matched -- which cannot encode object arrays and truncated first. - Two component ids where one path is a parent of the other pass every validation, then fail inside create_dataset. The writer is now atomic: build, validate and serialize both documents, then write a temporary file in the destination's directory and os.replace it into place, unlinking the temporary on any failure. Path collisions are refused up front with a message naming both ids. PoseFileHDF5Adapter registers at priority 10 ahead of the dataclass adapter, following internal/prediction/hdf5.py. Also fixes the test that was supposed to catch this. It only constructed a PoseFile inside pytest.raises and never called write_pose_file, so the sentinel assertion was a tautology that would have stayed green with validation moved after the open. Seven tests in test_writer_safety.py now write a sentinel and assert it survives each failure mode. And while in the writer: chunks=True let h5py pick a shape that splits the trailing axes, measured at 563x read amplification for a one-frame read, contradicting the rationale in that same file. Chunks are now shaped along the frame axis and span the other axes whole, at gzip level 1 rather than 6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five silent-wrong-data findings. None raised, none produced a finding, all
returned something plausible.
- read_component(frames=...) sliced dimension 0 after only checking that
"frame" appeared somewhere in axes. On a validate()-clean file with
axes ("slot","frame") a two-frame window returned two slot rows across
all frames. It now slices whichever axis the manifest names frame, which
is what the ADR's clipping rule requires of any tooling.
- No read path validated the manifest, though parse_manifest documents
that it takes a validated document. Against this PR's own invalid
corpus, a shape mismatch was absorbed by re-deriving shape from the
array, and a 2-body-part skeleton against a 12-wide keypoint axis was
returned intact, so any consumer indexing body_parts mislabeled real
coordinates. Both now raise PoseFileError, as does a declared path that
resolves to a group rather than a dataset.
- The encoding declaration was hardcoded to dense on write and never read
back, so a schema-valid ragged or RLE file round-tripped to "dense" with
its offsets dropped and no finding either side -- unrecoverable while the
bytes are still run values. Component now carries encoding; the writer
refuses what it cannot produce and the reader refuses what it cannot
decode, both naming the encoding.
- A round trip dropped /attachments, the manifest's attachments array and
every `extra` object, and relocated any payload not stored at its
id-derived path. The ADR calls dropping an attachment a specification
violation, and `extra` is the manifest's only forward-compatibility
mechanism. PoseFile now models attachments and extras, Component keeps
its stored path, and PoseFile's "full contents" docstring is true.
This also closes the "attachments cannot be declared" gap reported in the
PR description: a declared attachment no longer triggers the
attachment_undeclared warning.
Reader robustness while here: string attributes are decoded before
comparison, since h5py returns np.bytes_ for the fixed-length ASCII
attributes that the C, Fortran, MATLAB and Julia APIs write by default --
comparing those to str rejected genuine files, defeating the
cross-implementation interop the specification exists for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…what it claims validate() is the tool aimed at untrusted files, so it must never raise -- and it did, on four inputs: a missing, malformed or wrong-typed /provenance (it re-read the dataset outside the guard that had already recorded the problem), an axes/shape arity mismatch (it indexed shape by a position from axes right after recording that they disagree), a shape-(1,) integer schema_revision, and an /attachments *dataset* (the group test was a name test). Both JSON documents are now parsed once behind one guard, an arity mismatch stops that component, attribute coercion cannot raise, and the attachments walk checks the node is a group. A missing schema_revision no longer aborts the pass either: it says nothing about whether the payloads are sound, and skipping every payload and reference check because of it hid three defects behind one finding. Six checks were weaker than the specification text they cite: - strictly-increasing sparse indexes used np.diff, which wraps on unsigned dtypes -- so a decreasing uint32 index validated clean, and uint32 is exactly what the ADR prescribes for a frame index. Values are cast to int64 before differencing, and each index is checked once rather than once per referencing component. - skeleton edge bounds were never checked, so validate() certified files that read_pose_file then refuses. - a mask or length reference only had to exist, not to have a compatible shape; it must now align with the leading axes of what it describes. - a sparse index could be any component, including a frame-axis one. - an axis named frame was never checked against dimensions.frame, and dimensions.frame was never reconciled with video.frame_count, so dimensions was decorative. - the manifest's own format and schema_revision were written but never compared with the root attributes, making the forward-compatibility warning bypassable. Also deduplicates two constants the review flagged as transcribed between modules: the reserved namespace now has one definition in types, and the HDF5 string-attribute decoder is shared with the reader rather than copied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**String components.** DTYPE_NAMES silently narrowed the schema's dtype enum by omitting "string", and no numpy dtype yields that name -- so jabs.identity.external_ids, a component the ADR's own catalog defines, was unwritable, unreadable and falsely reported invalid. Component.dtype now maps object/bytes/unicode payloads to "string", the writer stores them as variable-length UTF-8 so any HDF5 implementation reads text rather than this machine's padding, and the reader decodes them back. **Validated mappings are now read-only views.** Frozen dataclasses freeze attribute bindings, not the objects bound, so `pose.dimensions["identity"] = 99` sailed past __post_init__ and was written to disk. dimensions, skeletons, missing and encoding become MappingProxyType, and axes is normalized to a tuple so a list does not compare unequal to what everything else uses. **Timestamps are checked.** `format: "date-time"` is advisory in jsonschema unless an optional validator package is installed -- it is not, so garbage validated clean. Checked explicitly instead, with no new dependency, over the manifest's created, video.start_time, and every provenance created and time. **The corpus is reproducible.** `created` was a wall-clock stamp with no way to pin it, so one regeneration run gave the three valid fixtures three different timestamps. write_pose_file takes an optional created, and the generator pins it; two runs now produce byte-identical files. The generator also copies its clean base before injecting the undeclared attachment, which had been giving all four invalid fixtures an unrelated attachment_undeclared warning. **The JABS skeleton is derived, not transcribed.** It was hand-written twice and duplicated KeypointIndex and FULL_CONNECTED_SEGMENTS with a case mismatch. skeletons.jabs_mouse12() builds it from jabs.core, expanding the polylines into deduplicated edges, and the fixtures and conftest both use it. One review point is answered rather than changed: build_manifest deliberately lets the writer's layout override a caller's. Layout describes the storage a file really has, so the writer's decision is the truth and a caller's preference is an aspiration -- now said so in the docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… pass Copilot reviewed d6a8520, before the four fix commits. Ten of its fourteen findings were already closed there; these four were not, and each was reproduced at HEAD before being fixed. **The writer could emit a file the validator rejects.** validate() gained frame_axis_length and dimensions_match_video, but nothing checked them at construction, and write_pose_file only runs the schema -- so a PoseFile with a 5-frame axis and dimensions.frame=4 was written happily and then reported unclean. PoseFile now checks every axis that names a dimension, plus dimensions.frame against video.frame_count, so the writer cannot produce a file its own validator refuses. **The schema helpers ignored the format they declare.** jsonschema treats `format` as an annotation unless a checker is supplied, so validate_manifest accepted created="not-a-date" while the shipped schema said otherwise. A date-time checker is now registered locally -- no new dependency -- so direct callers of the helpers get the constraint too. validate() classifies format failures under their own check name rather than as a schema failure, because a schema failure aborts the structural pass and one bad timestamp should not hide a missing payload. The standalone timestamp pass added earlier is now redundant and gone. **Non-dense offsets were never validated.** The ADR carries two error rows for group_offsets and instance_offsets, and they are checkable whether or not this build can decode ragged or RLE -- a conforming file is conforming either way. Both are now checked for presence, non-decreasing order, starting at zero, the correct terminal value, and instance_offsets having frame*slot+1 entries. Offsets are read as int64 first: they are uint64 by specification, which is the same wrap that let a decreasing frame index pass. **Only half the layout was compared.** Declared chunks and compression_opts are part of the layout schema and were ignored, so a manifest could claim any chunk shape or gzip level. Every declared key is now compared against the dataset, and the warning names which ones disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…raise Fifteen findings, each reproduced at HEAD first. Also re-extracts the two schemas after the ADR's round-2 amendments, so the shipped schemas and the specification stay in lockstep, and enforces the new conditionals in the domain types. **Interop.** attr_text recursed forever on a non-string root attribute, so `jabs_format = 1` raised RecursionError out of both the reader and the validator instead of being rejected. It now unwraps exactly one level. **String payloads were mangled.** The writer converted them with `[str(v) for v in arr.tolist()]`, which stringifies whole rows of a 2-D array into single elements -- a (2,2) array became shape (2,) with the element `b"['a', 'b']"`, a file the validator then rejected -- and turned `b"a"` into the literal text `"b'a'"`. Conversion is now elementwise with the shape preserved and bytes decoded, and the string branch honours the same chunk and compression layout as numeric components, which it had been skipping while the manifest declared it. Reading them was inconsistent too: a frame window bypassed the decode, so a string component handed back bytes for a window and str for a whole read. **The writer now validates the finished file** before os.replace. Only the two JSON documents were checked before, so file-level invariants -- offset integrity, sparse index ordering, layout agreement -- were still writable, and the review rightly pointed at two of this package's own tests that wrote a file its validator rejects. Those tests now corrupt the file after writing, which is the only way to build such a fixture. **validate() no longer raises on the files it exists to diagnose**: a truncated or non-HDF5 file returns a `file_readable` finding instead of OSError, and a JSON-object provenance with non-dict `records` no longer reaches `set(1)`. **Three checks were narrower than their invariant.** Only the frame axis was compared against dimensions, though the rule covers every named dimension and PoseFile checks them all. Mask compatibility compared shapes but not axis names, so a (frame,) payload could be masked by an (identity,) component of the same length. And a sparse index was cast to int64 before its dtype was checked, which crashed on a string index and silently truncated a float one into apparently valid frame numbers. **Construction now enforces what the schema does**, so the writer cannot emit a file its own validator rejects: keypoint components name their skeleton, a nan policy needs a float payload, an RLE payload is an unsigned integer, and skeleton width, sparse index axes and length, and mask axis alignment are all checked across components. **Round-trip fidelity.** Provenance `extra` on records, history entries and the document itself was silently dropped, contradicting the lossless contract. The types carry it and both paths preserve it. **Timestamps.** `fromisoformat` accepts a bare date and a local time with no offset, neither of which is an RFC 3339 date-time; both are now rejected. And `jabs.io.save` discarded its kwargs, so `created` silently used the wall clock -- options are forwarded, and an unsupported one fails visibly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-extracts the schemas after the ADR's round-3 amendments and implements the checks they added, in the domain types where a rule is self-contained and in validate() for files another producer wrote. - A `coord` axis must be exactly 2 wide. `coord_order` describes two values, so a component whose shape ends in 3 could not be read as xy or yx however well it validated. - A `mask` reference must be boolean and a `length` reference a non-negative integer no larger than the axis it bounds. Shape compatibility was the only test, so a float mask or a negative count passed while being inapplicable — alignment is not interpretability. - Offset datasets must be unsigned integers, as the encoding text always required. A float offset cannot index an array and a signed one admits negatives; both previously reported as "missing", which was a misleading message for a dataset that was present. - An RLE payload declares a single `run` axis, enforced by the schema conditional, so a `[N, 2]` payload can no longer coexist with offsets that terminate at `shape[0]`. - Attachments are checked in both directions: a declared attachment must exist, live under `/attachments/`, and be declared once. The asymmetry is deliberate and recorded in the ADR — an undeclared payload is a warning because it loses nothing, while a declaration with no payload is an error because a copy tool is required to preserve something absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8fa7038 to
20e2444
Compare
Stacked on #445 (ADR 0002). Base is
docs/adr-0002-pose-format, so this merges after the ADR. Review the ADR first — this PR is the ADR made executable.What this implements
The revision-1 format core in a new self-contained package,
jabs.io.internal.pose_file:schemas/*.jsonschema.pyvalidate_manifest,validate_provenancetypes.pyComponent,Skeleton,VideoInfo,PoseFile, provenance recordsmanifest.pywriter.pywrite_pose_filereader.pyread_pose_file,read_component(whole or by frame window),read_manifestvalidate.pyvalidate→list[Finding], implementing the ADR's validation tabletests/data/pose-format/Dense encoding only. The ADR makes the baseline mandatory and
ragged/rleoptional, so building all three now would be speculative.No existing behavior changes
PoseHDF5Adapter.readwas alreadyNotImplementedErrorbefore this branch.writegainslegacy=Noneto select the new format, which raisesNotImplementedErrornaming what is missing: thePoseData↔PoseFilemapping.PoseDatais identity-major with no slots and no provenance, so doing that mapping badly here would be worse than not doing it — it lands with the JABS integration. Legacy V2 write is unchanged and still tested.Verification
Root suite 1059 passed;
jabs-io375 (299 at baseline);jabs-core88;jabs-behavior113; ruff clean over 461 files. Package suites are run individually — running several together tripsImportPathMismatchErroron the per-packagetests/conftest.py, which is why CLAUDE.md lists them separately.jabs-visioncould not be verified: neithertorchnor thejabs.visionpackage installs underuv sync --all-extras. This branch touches nothing in it.Every schema tightening from the ADR review round has a negative test. The manifest now rejects: no
video, nomissing, acoordcomponent withoutcoord_order,raggedwithoutgroup_offsets, asampleaxis withoutsparse, andsparsewithout asampleaxis.Also verified rather than assumed: the schemas ship in the built wheel (
uv build --package jabs-io, then inspected the archive) —uv_buildincludes package data with no configuration.Two gaps this work surfaced, for the ADR
sampleaxis, must name itself as its own index. The ADR's dynamic-objects table shows no such self-reference.valid-sparse.h5uses self-reference because that is what the shipped schema demands. Either the table gains it or the rule is relaxed for index components. This wants a decision.attachmentsarray butPoseFilehas no attachments field, so the writer cannot produce a declared one.valid-full.h5carries an undeclared attachment and the corpus asserts the specified warning, which flags the gap rather than hiding it.Extensibility, demonstrated
valid-full.h5carries anorg.example.lab.whisker_anglecomponent, and a test reads a frame window out of it without knowing anything about it — the ADR's claim that declared axes let generic tooling subset unknown data, made executable.Follow-on stacked PRs
PoseData↔PoseFileand aPoseEstimationreading the new format; answers open questions 6, 7 and 8, and is where the format gets tried for real.🤖 Generated with Claude Code