Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add swmr mode #45

Merged
merged 24 commits into from
Aug 7, 2024
Merged

Add swmr mode #45

merged 24 commits into from
Aug 7, 2024

Conversation

stephprince
Copy link
Collaborator

Fix #39.

These changes turn on SWMR mode as part of the startRecording process and check that public NWBFile methods that create new datasets/groups cannot be used when isRecording is true.

I checked that the file is accessible for reading by a separate process while open during the uswSWMRmode test - I'm not sure the best way to incorporate that in the tests - @oruebel any suggestions?

@stephprince stephprince marked this pull request as ready for review August 1, 2024 18:42
@stephprince stephprince requested a review from oruebel August 1, 2024 18:42
@oruebel
Copy link
Contributor

oruebel commented Aug 1, 2024

I'm not sure the best way to incorporate that in the tests

I can see a couple of options:

  1. Simply test that we can open the file again in "r" mode while the NWBFile is still open. I think this may not even need to be in a separate process.
  2. A more complete test would be to start a separate thread to read from the file while you are writing to it.
  3. Have a command-line script that you are calling to read from the file

I think checking for 1 should be sufficient in the tests. SWMR is a feature of the HDF5 library (not AqNWB), so this needs to be tested in the HDF5 library.

src/hdf5/HDF5IO.cpp Outdated Show resolved Hide resolved
@oruebel
Copy link
Contributor

oruebel commented Aug 1, 2024

I think SWMR mode requires some user documentation, both because users are likely not familiar with this and because it provides AqNWB a set of unique features that have been called by the community as problems (in particular avoiding corruption of HDF5 files and being able to read while recording.

I filed a separate issue for this #48 to make sure we document: a) what it is, b) what it means for the user (i.e., we can read during recording and avoid file corruption), c) what the use pattern of the API is.

src/nwb/NWBFile.hpp Outdated Show resolved Hide resolved
src/BaseIO.hpp Show resolved Hide resolved
tests/testHDF5IO.cpp Outdated Show resolved Hide resolved
tests/testHDF5IO.cpp Outdated Show resolved Hide resolved
tests/testHDF5IO.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I think the main code item is to move IO specific properties to the IO backend to avoid backend-specific features in the NWB classes, i.e.,
- track the isRecording status in the IO backend
- have a way for the IO backend to say if objects can be created or not

src/hdf5/HDF5IO.cpp Outdated Show resolved Hide resolved
src/BaseIO.hpp Outdated Show resolved Hide resolved
tests/testNWBFile.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

I added few more comments, but overall this looks good.

src/BaseIO.hpp Show resolved Hide resolved
@stephprince
Copy link
Collaborator Author

We discussed offline that we wanted to implement the following changes, these should also fix #47 by making swmr mode optional.

  • Have HDF5IO take an optional disableSWMRMode argument (default false)
  • Have startRecording and stopRecording detect whether SWMR mode is enabled
  • Change pauseRecording back to stopRecording. We will likely not have full pauseRecording capabilities in swmr mode (i.e. the ability to pause, add additional objects, and restart the recording) until we implement the read side of the API.
  • Check in other io functions whether the file is open before performing any tasks

I have left out any changes to NWBRecording inputs for now, since we are still deciding the structure of NWBRecording vs. NWBFile.

src/hdf5/HDF5IO.cpp Outdated Show resolved Hide resolved
src/hdf5/HDF5IO.cpp Outdated Show resolved Hide resolved
src/hdf5/HDF5IO.hpp Outdated Show resolved Hide resolved
tests/testHDF5IO.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

A couple of minor additional comments but otherwise this looks good

@stephprince
Copy link
Collaborator Author

Finished updating based on the latest suggestions!

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your patients!

@stephprince stephprince merged commit e36766f into main Aug 7, 2024
8 checks passed
@stephprince stephprince deleted the add-swmr-mode branch August 7, 2024 21:01
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.

Add support for Single Writer Multiple Reader (SWMR)
2 participants