Respect zero per-dataset chunk cache settings - #2986

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.62%. Comparing base (0f9cbe7) to head (b39ee2d).
⚠️ Report is 11 commits behind head on master.

@@            Coverage Diff             @@
##           master    #2986      +/-   ##
==========================================
+ Coverage   90.51%   90.62%   +0.11%     
==========================================
  Files          16       16              
  Lines        2551     2561      +10     
==========================================
+ Hits         2309     2321      +12     
+ Misses        242      240       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Hi @rioyu123,

Thanks for the PR. any([efile_prefix, virtual_prefix, rdcc_nbytes, rdcc_nslots, rdcc_w0]) has always conflated "not given" with "given as zero or empty", and switching to explicit is not None checks is the correct fix. I checked the new tests against the merge-base and 21 of the 39 cases fail there, so they genuinely capture the problem. I would like two small things before I merge it, and then there is a larger piece that I will pick up myself.

1. An else in the cache test loop.

   In test_dset_chunk_cache_zero_values you open the file with rdcc_nbytes=1048576, rdcc_nslots=521, rdcc_w0=0.75, but the loop at h5py/tests/test_dataset.py:2218-2220 only checks the entries that were passed in, so nothing is watching the others:

   file_cache = f.id.get_access_plist().get_cache()[1:]
   for index, option in enumerate(('rdcc_nslots', 'rdcc_nbytes', 'rdcc_w0')):
       if option in supplied_options:
           assert cache[index] == supplied_options[option]
       else:  # <-- add
           assert cache[index] == file_cache[index]  # <-- add

   That will fail on this branch for zero_preemption, positive_preemption and mixed_values, which is the point explained below. Please add it as xfail for now, with a reference to this discussion if you want so the behaviour is pinned and we do not lose track of it.

2. A sentence in the news entry. Something to the effect that supplying any one of rdcc_nslots, rdcc_nbytes or rdcc_w0 currently resets the other two to the HDF5 defaults rather than leaving the file's values in place. As written the entry reads as though nothing else changes, and for rdcc_w0=0 something else does.

Follow-up work – not yours

I became aware while reviewing this PR that more work is needed but it does not belong here.

The chunk cache settings are built in three steps: (1) read whatever the DAPL currently holds; (2) overwrite with the entries the caller supplied; (3) set all three back. These steps look simple and safe, but the first step is actually problematic.

A DAPL that has just been created does not hold numbers for these three properties. It holds sentinels, and a sentinel means: I have nothing to say about this one, use whatever the file was opened with. Libhdf5 honors that when the dataset is opened: H5D__chunk_init() looks at each of the three separately and falls back to the file's own setting for any that is still at its sentinel.

The catch is that reading those sentinels back out again is not possible. H5Pget_chunk_cache() never reports one. For any property that was never set, it quietly substitutes a value from the library's default file access property list — not from the file actually opened. For libhdf5 2.2.0 that means: (8191, 8388608, 0.75). So the numbers that come back are not the sentinels that were stored, and setting them back replaces "ask the file" with three deliberate choices. By the time the dataset is opened there is nothing left for the fallback to do.

To fix what I just described requires a follow-up because the fix needs decisions that are not really appropriate for this PR. So: add the else branch and the news sentence and I will merge this.

Thanks @ajelenak. I've added the inheritance assertions and the release-note caveat. The explicit-option and data round-trip checks run before the xfail is applied, so they still fail normally: with the production fix reverted, all 21 original regressions fail. On HDF5 2.2.0, the three cases you identified account for the nine expected failures across the three dataset paths.

One note from testing: when the cache is disabled with zero bytes or zero slots, HDF5 reports both sizes as zero. Those cases keep their explicit-option and round-trip checks but skip the inheritance assertions.

Full suite: HDF5 2.2.0 gives 930 passed, 47 skipped, 9 xfailed; HDF5 1.10.10 gives 901 passed, 85 skipped, with all 39 focused cases passing normally. As requested, the inheritance fix itself stays out of this PR.

Thanks @rioyu123. I'll merge this on October 30 unless there are further comments from other maintainers.