Repository navigation
Tests and Benchmarks for regression - #85
Draft
yohannmarguier wants to merge 105 commits into
Draft
yohannmarguier wants to merge 105 commits into
yohannmarguier wants to merge 105 commits into
Conversation
Implements Step 1 (the "scaffold slice") of TEST_STRATEGY.md: build the machine on one example so every mechanism is proven before the matrix is broadened. Binds only to the public C ABI (al_lowlevel.h + al_const.h) per decision D1. What lands: - tests/contract/al_contract.h — reusable harness: RAII temp pulse dir, legacy-parameter URI builder, and typed round-trip helpers over al_write_data/al_read_data honoring the buffer-ownership contract (scalar caller-owned; char-array core-allocated + freed). - test_roundtrip.cpp — TEST_P write->read round trip on HDF5 AND Memory; oracle is round-trip self-consistency, needing zero DD artifacts (D5). - test_known_defects.cpp — the al_setvalue_*_parameter_plugin unregistered-name null-deref (FUNCTIONALITY_INVENTORY.md:410) encoded per D2: a DISABLED_ test asserting the CORRECT contract (returns an error), plus a companion death test pinning today's crash to SIGSEGV specifically. The bug is not frozen as spec; the day it is fixed the xfail flips to pass. - CMake wiring (D3): GoogleTest via find_package-or-FetchContent (pinned to the v1.15.2 commit), gtest_discover_tests with a "contract" label, and an AL_CONTRACT_SANITIZE toggle (D7) that instruments both the tests and libal with ASan/UBSan — non-blocking. The pure-C canary (testlowlevel_c.c) is retained unchanged. Deferred (recorded): D7's dedicated sanitizer CI leg + suppressions file, and the unit/integration label split, arrive with the broader matrix in later steps. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Step 3 of TEST_STRATEGY.md §4 (the thin unit tier), building on the #2 scaffold. Binds only to the public C ABI per decision D1. What lands: - test_introspection.cpp — pure-lookup unit tests for Cluster 4 (FUNCTIONALITY_INVENTORY.md:434-459), all intended-contract (D2): const2str/err2str on mapped ids; the silent "" for unmapped ids asserted as the deliberate contract (incl. the documented TIMERANGE_OP / FLEXBUFFERS_BACKEND map-coverage gaps, :75-79); the two maps proven distinct; getALVersion == the compiled AL_VERSION macro and dotted-numeric; getDDVersion == "!!DEPRECATED!!". - test_version_sentinel.cpp — decision D5 tripwire: a fake DD version written to ids_properties/version_put/data_dictionary round-trips verbatim AND leaves getDDVersion() unchanged, proving the core interprets no DD version. Parametrized over Memory (hermetic) and HDF5 (on-disk). - TRACEABILITY.md — decision D8 matrix; Cluster 4 + version-sentinel rows filled, scaffold rows seeded, remaining clusters listed as gaps for later build-order steps. - CMakeLists.txt — wires both files into contract_tests. The finer CTest unit/integration LABEL split (D6) stays deferred as the scaffold recorded; the pure-lookup subset is addressable via `ctest -R Introspection`. Per the issue's adapted TDD loop, each assertion was mutation-verified to fail for the right reason (real value surfaced) before setting the true expectation green. No new expected-fails; the crash-class defects remain in test_known_defects.cpp. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Implements Step 2 of TEST_STRATEGY.md — the bulk of the storage contract — building on the scaffold (issue #2) and binding only to the public C ABI (D1). Fixture layer (al_contract.h): - Deterministic synthetic-data generator over the datatype × shape space: synth_value/synth_buffer for CHAR/INTEGER/DOUBLE/COMPLEX and shape_for_rank scalar→7-D (MAXDIM), with values kept clear of the EMPTY sentinels so a scalar write is never silently dropped by data_has_non_zero_shape. - Generic typed write_data/read_data honoring the two ABI ownership regimes (scalar caller-buffer; array core-allocated + freed). - build_uri gains a FLEXBUFFERS branch: getURIBackend (al_context.cpp:280) has no flexbuffers case though the parse side accepts the scheme, so the URI is hand-built (documented, and tracked as an expected-fail). Matrix (test_roundtrip_matrix.cpp): - One TEST_P over Combine({HDF5,Memory,ASCII,Flexbuffers} × {CHAR,INTEGER, DOUBLE,COMPLEX} × ranks 0..7); each cell its own ctest entry. Oracle = round-trip self-consistency, zero DD artifacts (D5). - Each cell classified once (D2/D4): RoundTrip (assert equality), Refused (paired-negative — CHAR>2D on HDF5/ASCII; every Flexbuffers cell, which accepts the write but refuses the read-back as a serializer), or KnownDefect. - Known defects asserted as their correct contract and marked expected-fail: HDF5 crash on a CHAR scalar (DISABLED_ + paired death test), ASCII numeric corruption/abort at the MAXDIM boundary, and the flexbuffers URI-builder gap. TRACEABILITY.md: al_read_data/al_write_data rows filled (D8). Full contract suite: 139 passed, 4 known-defect cells skipped, 6 expected-fails disabled; each expected-fail verified to produce a real red when enabled. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
TEST_STRATEGY.md §4 step 4 (Cluster E): slice / timerange / al_list_filled_paths tested positive where the backend supports the capability and paired-negative (documented refusal) where it does not, so the supports* matrix is itself under test at the C ABI boundary (D1 — the C++ Backend flags are asserted through their sole ABI-observable consequence, accept-vs-refuse, since they cannot be linked directly). Support columns were established empirically against the binary and follow D2 (assert the real contract, not the issue's assumptions): - Memory genuinely supports slice (full SLICE_OP handling in memory_backend.cpp; correct closest-slice values), so slice is positive on HDF5+Memory and paired-negative only on ASCII+Flexbuffers — not the issue's assumed Memory-negative. - Refusal codes pinned specifically: timerange -> LOWLEVEL_ERR (al_lowlevel.cpp:1053), slice / list_filled_paths -> BACKEND_ERR, slice-read without an interp mode -> CONTEXT_ERR (al_context.cpp:347). What lands: - test_capability_gated.cpp — CapabilityMatrix TEST_P over the four always-on backends for timerange, slice read, slice-write-begin, and list_filled_paths; HDF5-only detail tests for the three interpolation modes (CLOSEST/PREVIOUS/LINEAR) and timerange with/without a resampling dtime grid, each with an exact value oracle (seeded via a single global write, so the read machinery is exercised independently of append). - KNOWN DEFECT per D2: repeated WRITE+UNDEFINED_TIME slices do not accumulate in this build (only the last persists). Encoded as a DISABLED_ correct-contract test asserting both slices persist plus a current-behavior tripwire pinning today's single-slice result; write lifecycle ownership stays with issue #3. - TRACEABILITY.md — Cluster E rows filled (covered + the append xfail); stale Cluster-E gap bullet removed. - CMake: test_capability_gated.cpp added to the contract_tests target. Full contract suite: 164 tests green (1 xfail disabled). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Correction of the .md to fit correctly in order of the processed issues.
What changed: tests/contract/test_capability_gated.cpp — open_seeded() refactored from a int-returning helper using non-fatal EXPECT_* into a void open_seeded(int* pctx) using ASSERT_*, with every call site wrapped in ASSERT_NO_FATAL_FAILURE(open_seeded(&pctx)). Why this fixes the defect: previously, if the URI build / data-entry open / seed write failed, the test continued and the paired-negative branch (EXPECT_NE(s.code,0) / EXPECT_EQ(s.code, BACKEND_ERR)) could pass for the wrong reason — a broken seed looking identical to a genuine documented refusal. ASSERT_* inside a subroutine only aborts the subroutine, so ASSERT_NO_FATAL_FAILURE at the call site is what actually aborts the test on setup failure. Now a refusal assertion can only be reached with a genuinely-seeded entry. The comment, which previously claimed fatal handling it didn't provide, now matches the code.
Two scaffold (issue #2) acceptance items were never delivered; this closes both. 1. ctest -L unit label split. The suite carried only the contract label, so ctest -L unit selected nothing (PRD user story 29). Now two disjoint gtest_discover_tests passes label each case by substrate (decision D6): unit = pure lookups + in-RAM Memory + plugin ABI tests (Introspection.*:*Memory*:KnownDefects*), integration = everything on-disk. Both also keep contract. Two gotchas handled: the LABELS list needs an escaped ; to survive re-emission, and gtest's negative-filter syntax is one - then colon-separated (*:-a:b:c), not -a:-b:-c. Guarded by CMake ≥ 3.22 (TEST_FILTER), falling back to the single contract label on the 3.21 floor. Verified: ctest -L unit = 49 hermetic tests (0 on-disk), runs in 0.20 s; integration = 126; contract = 175 (49 + 126, disjoint). 2. Non-blocking sanitizer CI leg + suppressions file. Added .github/workflows/sanitizers.yml: a Linux continue-on-error job that configures -DAL_CONTRACT_SANITIZE=ON, builds, and runs the contract suite under ASan/UBSan/LSan, with tests/contract/sanitizer_suppressions.txt wired via LSAN_OPTIONS. Validated the (previously never-exercised) AL_CONTRACT_SANITIZE build path locally: it compiles, links ASan into the binary, and passes 100% (163 tests) under ASan/UBSan. The run excludes Death tests (deliberate crashes) and the CurrentlyCorrupts tripwires — the latter because the ASCII rank-7 DOUBLE/COMPLEX corruption is UB that ASan's memory layout masks (documented in-code and in the workflow).
…tinel The sentinel test's real property is non-interpretation: writing a DD version to version_put/data_dictionary must not change what getDDVersion() returns. Compare the post-write value to the captured baseline (dd_before) rather than to the "!!DEPRECATED!!" literal — the literal value is already pinned by test_introspection.cpp::GetDDVersionReturnsDeprecatedSentinel, so this removes the cross-test coupling and keeps the invariance check correct even if the compiled sentinel ever changes. The one baseline assertion stays as the anchor. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Cover User Cluster 3 (plugin management) through the public C ABI, per TEST_STRATEGY.md build-order step 5 and decision D2. - test_plugin_fixture.cpp: a real, self-contained access_layer_plugin built as a loadable alcontract_plugin.so (the classic red-green fixture), so the register/bind/unbind/unregister state machine and the al_setvalue_* happy paths run end to end. Its setParameter logs the received value so tests can prove a value crossed the ABI, not just that the call returned success. - test_plugins.cpp: the register/bind/unbind/unregister state machine + the documented quirks (register-twice throws, bind-unregistered throws, double-bind throws, unbind-unbound silent no-op), al_is_plugin_registered, all three al_setvalue_* variants reaching the plugin, and framework gating. Two defects pinned as xfail + paired current-behavior tripwire: the registered-but-never-bound plugin left un-destroyed on unregister, and the dlopen-failure swallowed by the NDEBUG-stripped assert. - test_known_defects.cpp: add the double- and generic-variant setvalue null-deref death tests (SIGSEGV) + DISABLED_ correct-contract partners, joining the scaffold's int variant; share the unregistered-name constant. - al_contract.h: hoist the shared kUnregisteredPluginName constant. - CMakeLists.txt: build the plugin MODULE (POSIX-guarded), wire its dir/name via compile defs, add test_plugins.cpp, extend the unit label filter. - TRACEABILITY.md: fill the Cluster 3 rows, extend the xfail bookkeeping table (now 11 pairs), mark Part 3 setParameter covered (indirect). Empirical finding vs the inventory: the dlopen defect does not crash on this NDEBUG build (the null handle reaches dlsym, whose null-check throws), so it is a plain xfail, not a death test; the setvalue null-derefs genuinely SIGSEGV and remain death tests. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…le (issue #4) Characterizes existing C-ABI behavior for arrays-of-structures (top-level + nested write/iterate/read), al_delete_data at signal/structure/DATAOBJECT-root granularity, al_get_occurrences, and the remaining pulse-lifecycle detail (al_begin_dataentry_action modes, al_context_info, al_get_backendID, al_close_pulse CLOSE/ERASE) across the always-on backend tier. Adds a deterministic equilibrium-seed generator (scalar + timebase-carrying 2-D array + constraints AOS, oracle = content hash, no committed blobs) to exercise realistic AOS/timebase/HDF5-tensorization shapes. Empirically validated against the built library (not just source reading) per TEST_STRATEGY.md's TDD discipline, which surfaced several defects invisible to static analysis: ASCII's AOS read always reports size 0 (its READ_OP setup drains the stream into a lookup map before the AOS-size cursor ever runs), HDF5/Memory/ASCII/Flexbuffers each implement a disjoint, non-overlapping slice of the three delete granularities, ERASE_PULSE is a no-op-equivalent to CLOSE_PULSE on every always-on backend, ASCII's CREATE_PULSE silently overwrites instead of refusing, and al_get_backendID accepts a wrong-type context via an unchecked static_cast. Each genuine defect is pinned per decision D2: a DISABLED_ correct-contract test paired with an always-run current-behavior tripwire, never frozen as if it were the spec. Fills the AOS / delete / occurrences / lifecycle rows in TRACEABILITY.md. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Add structured-data contract tests: AOS, delete, occurrences, lifecyc…
Test/issue 7 plugin lifecycle
…sue #8) Pins caller-frees ownership for al_build_uri_from_legacy_parameters's *uri (the three other flagged APIs were already pinned by earlier issues), resolves empirically that plugin-parameter dim/size is not bounded by MAXDIM, and covers the remaining Plugin-author audience reachable through the C ABI: low-level al_plugin_* reentry, write/read data interception via a bound plugin, al_write_plugins_metadata, and al_bind/unbind_readback_plugins (extending the in-repo test plugin fixture with an env-var-gated readback capability). TRACEABILITY.md now has no blank rows; the two remaining gap rows are marked terminal with an explicit rationale. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Test/issue 8 ownership sweep
- TEST_STRATEGY.md D2 now states the actual (and better) discipline: each expected-fail's FUNCTIONALITY_INVENTORY / issue citation lives in tests/contract/TRACEABILITY.md as the single source of truth, and test files carry a pointer to the matrix instead of repeating citations in-line. - The three flagged test files (test_roundtrip_matrix.cpp, test_pulse_lifecycle.cpp, test_structured_data.cpp) each gained that one-line pointer in their header comment, so a reader landing on a DISABLED_ test knows exactly where its citation is.
Stands up a real, runnable MDSplus in an aarch64 Docker image (docker/mdsplus/), bakes a DD-4.1.1 model tree, and proves one equilibrium scalar survives a write->read round trip through the public C ABI against it -- lighting up the whole characterization stack (build gate, runtime skip, CTest label) before any behavioral breadth is attempted per issue #12. - tests/contract/test_mdsplus.cpp: MdsplusTracerBullet.ScalarSurvivesWriteThenRead, reusing equilibrium_seed.h's real DD path/value. Compiles out entirely unless AL_CONTRACT_HAVE_MDSPLUS is defined; GTEST_SKIP()s at runtime when MDSPLUS_MODELS_PATH is unset (TEST_STRATEGY.md D4: absent/skipped, never failed). - tests/contract/CMakeLists.txt: defines AL_CONTRACT_HAVE_MDSPLUS when AL_BACKEND_MDSPLUS=ON, and adds a new `mdsplus` CTest label. Also documents (without fixing, out of scope) a pre-existing gtest_discover_tests defect found while wiring this up: multi-element LABELS lists silently collapse to their first token, so `ctest -L unit`/`-L integration` currently select nothing -- the new mdsplus pass sidesteps it with a single-element list. - docker/mdsplus/: Ubuntu 24.04 (not the issue's suggested 22.04 -- MDSplus publishes no arm64 .debs for 22.04/20.04, empirically confirmed) with MDSplus, a JDK for the model-tree compiler, and the DD-4.1.1 bake via pip-installed imas_data_dictionary. Verified end-to-end: image builds, model bakes, contract suite builds with AL_BACKEND_MDSPLUS=ON, and `ctest -L mdsplus` passes. - tests/contract/TRACEABILITY.md: new Part 4 section (MDSplus backend, compile-guarded tier) with the four-status scaffolding and the tracer bullet's first row.
Adds MDSplus as a parameter/case to DataEntryModes, CapabilityMatrix,
Occurrences, and AosMatrix, reusing their existing test bodies, and
triages every resulting outcome into Part 4 of TRACEABILITY.md.
Empirically resolved against a real DD-4.1.1 model tree (docker/mdsplus/):
occurrence naming is "<ids>/<N>" (a third convention, same slash form as
ASCII); get_occurrences is real (gated on homogeneous_time being set);
list_filled_paths always throws (BACKEND_ERR); slice read/write genuinely
round-trip (supportsTimeDataInterpolation() is unconditionally true) while
timerange is refused (LOWLEVEL_ERR, supportsTimeRangeOperation() is
unconditionally false); and DataEntryModes' CREATE_PULSE guard matches
HDF5/Memory, not ASCII's defect.
AosMatrix and EquilibriumSeedMatrix hit the same divergence: MDSplus
resolves every path against its real DD-baked model tree, so their
synthetic/DD-agnostic field shapes ("elements"/"val", the seed's flat
profiles_1d/psi write, the generic constraints AOS) throw Node Not Found
at flush time. This is a legitimate storage-model difference, not a
defect, so AosMatrix's MDSplus case is skipped (via a new AosExpect::
Divergence, mirroring the existing KnownDefect skip) and MDSplus is left
out of EquilibriumSeedMatrix's backend list (like Flexbuffers already is,
for its own reason) rather than landing a permanently red test -- both are
documented in-place and in TRACEABILITY.md.
No genuine defect surfaced this round, so no new xfail pairs.
…ip logic - EquilibriumSeedMatrix now takes an actual MDSplus instantiation (skipped in-place via GTEST_SKIP) instead of omitting it from kSeedBackends[], matching AosMatrix's divergence treatment and issue #12 Q5's explicit listing of this fixture as one MDSplus can join. - Extract the repeated "skip if MDSPLUS_MODELS_PATH unset" block into a single AL_CONTRACT_SKIP_IF_MDSPLUS_UNCONFIGURED() macro in al_contract.h (a macro, not a function, since GTEST_SKIP()'s implicit return must unwind the calling SetUp()/TEST body directly); reuse it in test_mdsplus.cpp, test_pulse_lifecycle.cpp, test_capability_gated.cpp, and the new Occurrences test. - Add `divergence` to TRACEABILITY.md's status vocabulary list (it was used as a live status without ever being defined there). - Fix a stray escaped-slash in CMakeLists.txt's mdsplus TEST_FILTER that didn't match its own explanatory comment.
RoundTripMatrix sweeps {CHAR,INTEGER,DOUBLE,COMPLEX} x rank against synthetic
opaque paths MDSplus structurally can't join. test_mdsplus_real_paths.cpp
curates one real DD-4.1.1 path per (type, rank) cell instead, sourced by
walking build-mdsplus/_deps/data-dictionary-src/IDSDef.xml directly (the
imas-dd MCP tool wasn't available in this session), spanning multiple IDSs.
32 cells: 18 covered (13 plain leaf, 5 via 1-3 level AOS descent reusing
AosMatrix's proven idiom), 1 divergence (CHAR r0 against a real STR_0D node:
MDSplus's text node is inherently string-shaped, so it cleanly refuses a
dim=0 read-back instead of HDF5's crash on the same synthetic shape), 13
terminal-gap (DD 4.1.1 has no field of that type/rank anywhere, e.g. no
STR_2D+, no CPX_0D). Verified against a real MDSplus tree in docker/mdsplus/:
ctest -L mdsplus is green (45/45).
`al_delete_data`'s `path` is accepted by the ABI and dropped by the HDF5 backend: `HDF5Backend::deleteData` never forwards it and `HDF5Writer:: deleteData` has no parameter to receive it, so any non-empty path removes the whole IDS occurrence and reports success. Characterize that before touching it. The existing tripwire only observed the sibling leaf; widen it to the two consequences issue #63 measured as well — the occurrence's own `<ids>.h5` is deleted, and the master file is left holding an external link to a file that no longer exists (observed through `al_get_occurrences`, which enumerates those links live). Confirmed red/green today: the three correct-contract assertions fail, the tripwire passes.
Decision: `path` means what the ABI has always said it means — a non-empty path deletes that node and its subtree only, an empty path addresses the whole DATAOBJECT. Recorded in docs/adr/0001-al-delete-data-path-semantics.md together with the per-backend matrix and why the cheaper alternative (rejecting a non-empty path as a caller error) was rejected: it is not what the header says, not what the other backends do, and it would fail every non-slice `put()` and `delete_data()` through imas-python, whose traversal issues one delete per DD leaf. `HDF5Writer::deleteSubtree` selects the matching datasets out of the IDS group's flat namespace of tensorized names — the mangled path, its `_SHAPE` companion, and anything continuing with `&` (a structure child) or `[]` (an AOS index). The continuation character is what keeps `time` from taking `time_slice[]&x` with it. Buffered writes are flushed before anything is unlinked, since HDF5DataSetHandler::close() frees write buffers without writing them. The group listing is cached across a run of deletes, and invalidated by anything that can add a dataset, so the HLI's leaf-by-leaf traversal is not quadratic in the size of the IDS. `deleteOccurrence` keeps the empty-path behavior and fixes two defects in it: the master file's external link is now removed with the file it points at (it used to be left dangling, so al_get_occurrences kept reporting an occurrence whose file was gone), and a closed-but-present IDS file is deleted instead of being silently reopened and leaked. Tests: DeleteMatrix's HDF5 leaf/structure cells are enabled, the HDF5 xfail pair retired, and the layout-specific consequences the backend-agnostic matrix cannot see are pinned in Hdf5Delete.* — which files and master-file links survive which granularity, same-prefix siblings, an absent path, and a run of successive leaf deletes all taking effect. Verified against the suite, the Debug leg, and ASan/UBSan.
The first cut matched dataset names by string prefix plus a continuation character. That is right for every path the HLI issues, but it matches nothing at all for a path that traverses an array of structures: `outer/inner` mangles to `outer&inner` while the dataset is named `outer[]&inner&leaf`. The call then deleted nothing and reported success — in miniature, the same "argument accepted, ignored, success reported" state this issue exists to remove. Confirmed by test before changing the rule. Match per segment instead: each segment of the requested path matches a dataset-name segment with or without its `[]` AOS suffix, and the name's remaining segments are its subtree. Segment-wise still tells a real child from a same-prefix stranger (`time` vs `time_slice[]&x`, `code` vs `code_name`), and it gives an AOS-crossing path the one meaning the ABI leaves available: `deleteData` receives an OperationContext, never an arraystruct one, so no element can be named and the path can only mean "that subtree in every element" — exactly what the tensorized layout holds.
Standards axis: - The group listing now validates itself against the group's live link count instead of relying on four hand-placed invalidateGroupMembers() calls on the write paths. Only a write can add a link, and a write can only add, so an unchanged count means an unchanged set of names — which removes the latent coupling where any future dataset-creating path had to remember to invalidate. - unlinkFromGroup() owns both the H5Ldelete loop and keeping the cache in step, so deleteSubtree no longer reassigns the writer's own vector through a returned reference. - The delete error builds its message as a std::string rather than through a fixed-size buffer: a dataset name is a caller-supplied DD path, which the src/hdf5 `char error_message[200]` idiom cannot bound. - Both H5 failures in the listing name their own function. - Hdf5Delete's tests all go through the fixture now; the free observation helper that duplicated three of its members (and skipped make_legacy_tree, unlike every other on-disk path in the file) is gone. Spec axis: - al_lowlevel.h's per-backend summary said memory does "node and subtree" where only the AOS case is characterized, and omitted UDA entirely. - The ADR records the two behaviour changes the issue did not itself measure — master-link removal on an empty-path delete, and deleting an IDS file that is closed but still on disk — rather than leaving them silent. The one substantive fidelity finding, that a path below an AOS silently matched nothing, was already fixed in the preceding commit; the review confirms it independently.
Make HDF5 al_delete_data honor its `path` argument (issue #63)
`al_read_data` has always documented that a fieldpath may address a field from the dataobject root with a prepended `/`, but `HDF5Reader::read_ND_Data` honored only the relative half: it replaced `/` with `&` and then unconditionally prepended the current AOS context's tensorized path, so an already-rooted name was appended to the context it was meant to replace (`time_slice[]&constraints&j_phi[]&&time_slice&constraints&j_phi&…`). `H5Lexists` found nothing, the reader returned 0, and `al_plugin_read_data` filled the caller's buffer with EMPTY_* while reporting code 0 — stored data reported as absent, a silent wrong answer rather than an error. Decision: an absolute path resolves **by path segment** against the AOS chain the caller has open, cutting at the last matching AOS boundary. That answers the two coupled questions together — which dataset name to look up (the tensorized `[]` markers of the retained levels are kept) and which indices still apply (only the retained levels', so a child's cursor is never applied to an ancestor's shallower dataset, and the timed-AOS designation is dropped with it). Recorded in docs/adr/0002-absolute-field-path-resolution.md with the four rejected alternatives — chiefly plain de-slashing, which fixes only root fields, and keeping the child's whole cursor, which returns a wrong number instead of none. Segment-wise matching is the same rule ADR 0001 chose for `al_delete_data`, for the same reason: it tells a real descendant from a same-prefix stranger. The resolution lives in `HDF5Utils::resolveAbsoluteFieldPath`, which reports how many AOS levels the target still sits under; `read_ND_Data` truncates the current indices to it. No DD-version knowledge, no schema lookup, no ABI change. A target beyond an AOS the caller never opened has no cursor and no schema to infer one from, so it resolves without that `[]`, is not found, and travels the ordinary missing-data path — decided, and pinned by a test. Scope is HDF5 reads, as issue #65 establishes: the suite writes through relative spellings only, so it presumes nothing about writes, delete or arraystruct opening. Tests: Hdf5AbsoluteFieldPaths.* — both spellings of one address compared at nonzero indices on both levels of a nested AOS (time_slice → constraints/j_phi), for a scalar leaf, a CHAR array leaf, ancestor targets, root targets and the operation context itself; the plugin twin (al_plugin_read_data) asserted separately; cache/iteration stability, an absent-data control, and the unopened-AOS limit. Ten of the thirteen fail on the parent commit. Verified against the full suite, the Debug leg, ASan/UBSan (no new findings) and the Python suite.
…-reads Make HDF5 reads honor absolute field paths (issue #65)
yohannmarguier
requested review from
SimonPinches,
olivhoenen and
prasad-sawantdesai
as code owners
September 22, 2026 15:51
SimonPinches
left a comment
SimonPinches
left a comment
Contributor
There was a problem hiding this comment.
Too large to review in one go, but here are a few initial comments.
| pull_request: | ||
|
|
||
| env: | ||
| DD_VERSION: "4.1.1" |
Contributor
There was a problem hiding this comment.
How do we see this version being updated following future DD releases? Or do we want DD_LATEST ?
| python -c "import sys, pprint, cibuildwheel; pprint.pp(sys.path); pprint.pp(cibuildwheel)" | ||
| python -m cibuildwheel --output-dir wheelhouse --config-file pyproject.toml | ||
| env: | ||
| UDA_REF: "2.8.1" # keep in sync with windows_deps.env.UDA_REF |
Contributor
There was a problem hiding this comment.
Should the filename not capture the exact model of laptop and perhaps the OS it runs, rather than who owns it?
| hard-coded into the script or this doc: | ||
|
|
||
| ```sh | ||
| salloc --exclusive --partition=<PARTITION> --time=00:30:00 |
Contributor
There was a problem hiding this comment.
Do we scan over partitions = hardware generation?
|
|
||
| ### Issue tracker | ||
|
|
||
| GitHub Issues on the `origin` fork `yohannmarguier/IMAS-Core` (via the `gh` CLI); external PRs are not a triage surface. See `docs/agents/issue-tracker.md`. |
Contributor
There was a problem hiding this comment.
Should this be:
Suggested change
| GitHub Issues on the `origin` fork `yohannmarguier/IMAS-Core` (via the `gh` CLI); external PRs are not a triage surface. See `docs/agents/issue-tracker.md`. | |
| GitHub Issues on the `origin` fork `iterorganization/IMAS-Core` (via the `gh` CLI); external PRs are not a triage surface. See `docs/agents/issue-tracker.md`. |
1 of 5 tasks
olivhoenen
requested changes
Oct 2, 2026
Collaborator
There was a problem hiding this comment.
Thanks @yohannmarguier, indeed this can't go in as one big PR, it needs to be structured better. At least we need to:
- divide it in smaller PRs each on a more focused topic (feature, bugfix, optional additions for build/docker, agentic info, etc...)
- avoid long LLM comments referring to an issue in your fork, as they don't correspond upstream (e.g. https://github.com/iterorganization/IMAS-Core/pull/85/changes#diff-13984fccda0a0e26ddbd5148b60522195a915142cfc026c5121342254d0d7835R3, but basically everywhere)
- there are many changes in HDF5 backend, please create issues upstream for identified bugs before creating the PRs with potential fixes/improvements (cf. first point above)
This was referenced Oct 14, 2026
Collaborator
olivhoenen
marked this pull request as draft
October 15, 2026 15:35
This branch has not been deployed
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.
Markdowns in 'benchmarks' and 'tests' directories provide description of the coverage and the uses