Skip to content

Change H5ContextManager to use temp files and atomic operations#1660

Open
tskisner wants to merge 3 commits into
masterfrom
tsk/hdf5_atomic
Open

Change H5ContextManager to use temp files and atomic operations#1660
tskisner wants to merge 3 commits into
masterfrom
tsk/hdf5_atomic

Conversation

@tskisner

@tskisner tskisner commented Jun 11, 2026

Copy link
Copy Markdown
Member

Change H5ContextManager to use temp files and atomic operations

  • Remove '*args' from H5ContextManager. The h5py.File constructor
    does not take positional args, so there is nothing to pass. Explicitly
    parse and handle the mode keyword arg and check that the file
    exists if required.

  • Unless mode="r", open a temporary file for modification. Add a
    classmethod to close an hdf5 file handle, optionally moving the temp
    file into place with a rename.

  • Implement H5ContextManager tests, including for case of replacing
    file while previous inode is open.

  • Update use of H5ContextManager throughout sotodlib, ensuring that
    the classmethod close() is used, which moves the temp file into place.

- Remove '*args' from H5ContextManager.  The h5py.File constructor
  does not take positional args, so there is nothing to pass.  Explicitly
  parse and handle the `mode` keyword arg and check that the file
  exists if required.

- Unless mode="r", open a temporary file for modification.  Add a
  classmethod to close an hdf5 file handle, optionally moving the temp
  file into place with a rename.

- Implement H5ContextManager tests, including for case of replacing
  file while previous inode is open.

- Update use of H5ContextManager throughout sotodlib, ensuring that
  the classmethod close() is used, which moves the temp file into place.
@tskisner tskisner marked this pull request as ready for review June 11, 2026 19:50
@tskisner tskisner requested review from mhasself and mmccrackan June 11, 2026 19:57
@tskisner

Copy link
Copy Markdown
Member Author

The strange MPI unit test hang / deadlock is reproducible locally. It is not related to this PR, but will work on fixing it.

@mhasself mhasself left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for figuring this out! @mmccrackan should give the final approval, but this seems like a huge move in the right direction.

@tskisner

Copy link
Copy Markdown
Member Author

The unit test for these changes is not working as intended (using multiprocessing inside the unit test). I don't think it impacts the conclusions, but I'm going to push an update that moves the test to an independent script.

@tskisner

Copy link
Copy Markdown
Member Author

The testing of inode preservation when the file is changed is now in a standalone script whose main is called in the unit tests. You can also observe the failure of the test in master branch by installing master and running the script from this branch:

./tests/test_hdf5_atomic_update.py

and confirming that the code times out trying to access the HDF5 file. Then observe how that same executable works with the code in this branch.

@mmccrackan , thanks for any feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants