Usage of ancient files for testing breaks with recent h5py/libhdf5

[kmuehlbauer]

While working on #102 @bnlawrence and I found a glitch when we recreated test files with recent h5py/libhdf5.

See #102 (comment) for details.

Citing here for visibility:

If we recreate the file now, the following line of code:

attrs['vlen_str_array'] = [b'Hello', b'World!']

translates to this HDF5 dump:

ATTRIBUTE "vlen_str_array" {
   DATATYPE  H5T_STRING {
      STRSIZE H5T_VARIABLE;
      STRPAD H5T_STR_NULLTERM;
      CSET H5T_CSET_ASCII;
      CTYPE H5T_C_S1;
   }
   DATASPACE  SIMPLE { ( 2 ) / ( 2 ) }
   DATA {
   (0): "Hello", "World!"
   }
}

whereas the old file translates to:

ATTRIBUTE "vlen_str_array" {
   DATATYPE  H5T_STRING {
      STRSIZE 6;
      STRPAD H5T_STR_NULLPAD;
      CSET H5T_CSET_ASCII;
      CTYPE H5T_C_S1;
   }
   DATASPACE  SIMPLE { ( 2 ) / ( 2 ) }
   DATA {
   (0): "Hello\000", "World!"
   }
}

It might make sense to move from the current behaviour of testing already created files (and ingesting them into GIT) to creating test files only when testing in the test suite. This is what updownstream libraries like xarray and h5netcdf are doing. Thoughts?

[bnlawrence]

Thanks for a) realising what was going on, and b) making this suggestion.

I totally agree that in principle the right way is to generate the files on demand, but given one of our motivations here is to access data that has already been produced, I am a bit wary. There are two mixed issue here: 1) the h5py behaviour has changed on writing, and 2) we want to test our ability to read the data correctly. I guess we knew that the HDF5 library is a moving target, but the data on disk can't move.

That makes me wonder if we need a bit of both in our testing strategy: files we keep "in the repo" as they exhibit things we need to be able to read that are hard (or maybe even impossible now) to (re)create, and the rest should be generated on demand as you suggest. ("in the repo" in quotes because maybe we will keep them in S3 for testing).

I wonder if we need a bit of both, some files we keep for "curation testing" and the rest "on demand"?

(That begs the question as to how we would know when things should migrate from on demand to persisted, but I can't see how else we catch those but as a consequence to failures in our test suite over time, as we are seeing here.)

Thoughts?

[kmuehlbauer]

Thanks @bnlawrence for sharing your thoughts on that.

I believe that anything which was written to an hdf5 file in the past, can be (re)created with recent tools (namely h5py). But this we'll have to put to the test. And if this can be guaranteed for all times, is at least questionable. I therefore agree, that there can be good reasons to keep files for testing which have been created with old versions (or are just out there).

As a first step we might try to (re)create all existing files and see what else did change over time. For those possible changes we need to find the exact (re)creation incantations, which then should be fed into the new creation-while-testing behaviour. If we can't work out any (re)creation for certain feature would be an indication of keeping those files around.

[kmuehlbauer]

@bnlawrence

This just came to mind: Instead of changing the data files in #102 and #105, we might just keep those files and testing and add the creation on demand on top. That way we could slowly adopt the new behaviour. I can introduce the needed changes, if this would be OK with you.

[bnlawrence]

Thanks @kmuehlbauer. I agree that if we can keep the original files and add "creation on demand" over top, that's a reasonable strategy to get us past this point and so we can merge the other two pull requests. (It might be interesting to see what happens with having to support two versions of the vlen string testing file, but I think it should be ok.)

We can then keep this issue open and generate a "longer-term solution" which addresses "doing it properly" a bit further down the line.

[kmuehlbauer]

Just for the sake of completeness. The current layout of the tests as discussed above and first implementation in #102 and #105 is as follows:

1. We keep the current ingested files as is
2. We change the make files when introducing new test data, see https://github.com/NCAS-CMS/pyfive/blob/4512046c99ceb5d949f4cb87a70c2895bc7eee98/tests/make_attr_datatypes_file.py for an example
3. We adapt the tests with a fixture, which creates new test files on demand in the testsuite, see

   pyfive/tests/test_attr_datatypes.py

   Lines 18 to 24 in 4512046

   	@pytest.fixture(scope="module")
   	def attr_datatypes_hdf5(tmp_path_factory):
   	tmp_dir = tmp_path_factory.mktemp("attr_datatypes")
   	path = tmp_dir / "attr_datatypes.hdf5"
   	subprocess.run([sys.executable, MAKE_ATTR_DATATYPES_SCRIPT, str(path)], check=True)
   	return str(path)

4. We use that fixture to test against the new data in additional test functions, see

   pyfive/tests/test_attr_datatypes.py

   Lines 151 to 157 in 4512046

   	def test_empty_string_datatypes(attr_datatypes_hdf5):
   	with pyfive.File(attr_datatypes_hdf5) as hfile:
   	enum_attr = hfile.attrs['empty_string']
   	assert enum_attr == pyfive.Empty(dtype=np.dtype('|S1'))
   	assert enum_attr.dtype == np.dtype('|S1')

Note: The testsuite might not run properly locally when the test files are (re-)created, for obvious reasons.

[bnlawrence]

This is great. I'm thinking we probably need a "for maintiner's" section in our docs? (I am not suggesting you should do it now, but we could collectively do it at some future time.)

[kmuehlbauer]

I'm thinking we probably need a "for maintiner's" section in our docs? (I am not suggesting you should do it now, but we could collectively do it at some future time.)

@bnlawrence Yes, I agree, this should be the way forward.