-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add swmr mode #45
Conversation
I can see a couple of options:
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. |
I think 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. |
There was a problem hiding this 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
There was a problem hiding this 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.
We discussed offline that we wanted to implement the following changes, these should also fix #47 by making swmr mode optional.
I have left out any changes to |
There was a problem hiding this 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
Co-authored-by: Oliver Ruebel <[email protected]>
Finished updating based on the latest suggestions! |
There was a problem hiding this 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!
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 whenisRecording
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?