Conversation
joaopaulosr95
force-pushed
the
fix/preserve-reference-layout-on-dataset-get
branch
from
September 18, 2026 15:50
fa7bea6 to
0131551
Compare
joaopaulosr95
marked this pull request as ready for review
September 18, 2026 16:01
Contributor
Author
|
@mattjala this fixes a critical regression. We're currently running a custom build of this in production. |
mattjala
added a commit
to mattjala/hsds
that referenced
this pull request
Sep 22, 2026
Fork of HDFGroup#468 which adds a test, and fixes the unit test registration. testall.py registers dset_dn_test in unit_tests, which otherwise would never run it, and reflows that tuple to stay under the 99 character flake8 limit.
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.
Summary
Since
b6016e0("use hdf5-json util classes"),GET_Datasetinhsds/dset_dn.pyoverwrites the dataset's
creationProperties.layoutwith the top-levellayoutkey:Impact
Any dataset whose stored json has both keys — i.e. anything written by an older HSDS or by
hsload --link— silently returns zeros/empty strings instead of its data. Metadata, dtype, shape and attributes are all still correct, so it does not look like a failure.This covers NREL's public HSDS data on
s3://nrel-pds-hsds. Reading NSRDB through 1.0.x returns an all-zero array with no error of any kind; the same read on 0.9.4 returns the real series.Why CI didn't catch it
POST_Datasetin 1.0.x never stores a top-levellayoutkey (it logs"unexpected key for POST Dataset: 'layout'"). So for datasets created by this version"layout" in dset_jsonis False, the overwrite never happens, andtests/integ/dataset_test.py::testChunkedRefIndirectDataset— which does assert thatclass,file_uriandchunk_tablesurvive a GET — passes.The bug only reaches datasets in the older on-disk shape, which the test suite never constructs.
Reproducer
Against any 1.0.x server, using the public NREL bucket (no credentials needed):
H5D_CHUNKED_REF_INDIRECT(+file_uri,chunk_table)H5D_CHUNKEDair_temperaturemin/max-84 / 3770 / 0time_index[:3]b'2025-01-01 00:30:00', …b'',b'',b''The stored object confirms which one is right —
s3://nrel-pds-hsds/db/02161936-77b022e7/d/bb36-de0d1a-f7d448/.dataset.jsonhascreationProperties.layout.class == "H5D_CHUNKED_REF_INDIRECT"withfile_uriandchunk_table, and a separate top-levellayoutof{"class": "H5D_CHUNKED", "dims": [2000, 500]}. 1.0.x reports the latter, contradicting its own stored metadata.The change
GET_Datasetnow delegates to a newgetDatasetCreationProps()inhsds/util/dsetUtil.py, which keeps 1.0.x's behaviour of reporting the resolved chunk shape except when the creationProperties layout is one ofCHUNK_REF_LAYOUTS, where it is preserved intact.Two smaller things fixed along the way:
dset_json["creationProperties"]in place. Sincedset_jsonis themeta_cacheentry returned byget_metadata_obj, that corrupted the cached metadata for the lifetime of the process, not just the response. The helper returns a copy.CHUNK_REF_LAYOUTSis currently duplicated indset_lib.pyandchunk_crawl.py. I put the shared copy inutil/dsetUtil.py(a leaf module the DN can import —dset_libpulls inservicenode_lib, so it isn't importable fromdset_dn). Happy to follow up with a patch pointing the other two at it if you'd like that consolidated.Tests
tests/unit/dset_util_test.py::testGetDatasetCreationPropscovers:H5D_CHUNKED_REF_INDIRECT→ reference preserved (this is the regression)H5D_CHUNKED_REFandH5D_CONTIGUOUS_REFIt fails on
masterand passes with the change. Full unit suite: 87 passed, 1 skipped.flake8clean.Verification
Built a 1.0.1 image with the patch and read NSRDB through it against the real public bucket. The layout is reported correctly again, and a TMY CSV extracted through the patched 1.0.1 is byte-identical (sha256
7486b799…) to the same extraction through 0.9.4.