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

Expose H5PL low-level API #1256

Merged
merged 18 commits into from Jul 2, 2019
Merged

Expose H5PL low-level API #1256

merged 18 commits into from Jul 2, 2019

Conversation

takluyver
Copy link
Member

This is a continuation of #1166. I've updated it to use the @insubprocess decorator @scopatz added, fixed the tests running in a conda environment (conda-forge configures a non-default plugin directory when building HDF5), and added some docs for the new functions.

Remaining questions:

  1. Currently the path APIs accept and return bytes. Although this is technically correct for Unix paths, it's more common in Python to work with paths as strings. We're a bit inconsistent elsewhere - the low-level API for an external link returns bytes, but getting the source of a virtual dataset returns str. I'm leaning towards staying with bytes in the low-level API.
  2. Do we want to do any locking? On H5PL: Plugin Interface #1166, @tacaswell said they shouldn't need the phil lock, but there might be reasons to have a separate lock for the plugin search path:

We may also want to lock them between eachother (as I can imagine an append and a remove colliding and ending up with a "gap" in the hdf5 datastrucutres....).

I'm tentatively marking this for 2.10, but I don't mind if we bump it to a later release.

@takluyver takluyver added this to the 2.10 milestone Jun 29, 2019
@scopatz
Copy link
Member

scopatz commented Jul 1, 2019

This looks good to me! Thanks @takluyver!

there might be reasons to have a separate lock for the plugin search path

Does HDF5 have a lock of its own around this?

@takluyver
Copy link
Member Author

I can't see locking in a quick look at the HDF5 source code, though it's possible that I'm overlooking it in a macro or something. Maybe we should just document for now that it's very likely not thread-safe.

@scopatz
Copy link
Member

scopatz commented Jul 1, 2019

Maybe we should just document for now that it's very likely not thread-safe.

Seems reasonable for now

@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #1256 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1256   +/-   ##
=======================================
  Coverage   83.82%   83.82%           
=======================================
  Files          18       18           
  Lines        2170     2170           
=======================================
  Hits         1819     1819           
  Misses        351      351
Impacted Files Coverage Δ
h5py/__init__.py 59.64% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b887dac...7992db7. Read the comment docs.

@scopatz scopatz merged commit bfb6e72 into h5py:master Jul 2, 2019
@scopatz
Copy link
Member

scopatz commented Jul 2, 2019

Thanks @takluyver!

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.

None yet

3 participants