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

Concurrent access to PSpecContainer #193

Merged
merged 15 commits into from
Jul 10, 2019
Merged

Conversation

philbull
Copy link
Collaborator

Add a 'transactional' mode and enable HDF5's "single writer, multiple reader" (SWMR) mode to allow concurrent access to PSpecContainer objects.

The transactional mode is enabled when the container is instantiated with the keep_open=False kwarg. This keeps the HDF5 file closed except for when actions are being performed on it, when it is briefly opened and then closed again. This is helpful if multiple processes need to write out to the same file for example, as HDF5 files cannot be opened in read-write mode by more than one process.

The SWMR mode allows multiple processes to open the file for reading at the same time, as long as at most one process has opened it in read-write mode. Generally, the read-write process should open the file first; if a read-only version has been opened beforehand, opening in read-write mode may fail.

@ghost ghost assigned philbull Mar 25, 2019
@ghost ghost added the in progress label Mar 25, 2019
@coveralls
Copy link

coveralls commented Mar 28, 2019

Coverage Status

Coverage increased (+0.02%) to 96.657% when pulling 0e57b6a on parallel_safe_container into 4bb8d6e on master.

Copy link
Member

@plaplant plaplant left a comment

Choose a reason for hiding this comment

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

This looks like a great addition to the pspec container object. Most of my comments are minor, and are mostly stylistic.

I did have a couple of larger questions about how this application works within the SWMR paradigm. According to the SWMR user guide, the writer is only permitted to append data along an unlimited dimension, or modify existing datasets. It is explicitly not allowed to add new groups, datasets, attributes, etc. I am not very familiar with the typical access patterns of the pspec container object, but it seems like some object methods like set_pspec can create new groups and datasets. Clearly there are tests that are passing, so perhaps the SWMR feature is being used in such a way that it's compatible with the limitations of the HDF5 library. However, I am nervous about using the features in undocumented ways, which may lead to headaches down the road.

Additionally, use of the SWMR feature requires that the underlying file system be POSIX-compliant. A notable exception to this is NFS or other network-mounted filesystems. In the documentation of this parallel access feature on hera_pspec, the user should be informed of this requirement, so that it can be used appropriately.

hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/container.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Show resolved Hide resolved
Copy link
Member

@nkern nkern left a comment

Choose a reason for hiding this comment

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

This looks good to me and its passing the travis pull. Failing the travis push with some non-related OS errors.

@philbull philbull merged commit aca6586 into master Jul 10, 2019
@philbull philbull deleted the parallel_safe_container branch July 10, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants