Continues #48

[ocefpaf]

I also added the thread safe option to Windows.

Closes #48

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

[ocefpaf]

@marqh can you take a look if this implements everything you expect from #48?

[jakirkham]

Could someone please explain what this warning means for C++ support? A lack of understanding stopped this before.

#48 (comment)

[jakirkham]

To be totally clear, breaking C++ support is a blocking issue for me. So if this is not breaking C++ support I'm ok with it, but that needs to be demonstrated and/or backed up somehow.

[ocefpaf]

Could someone please explain what this warning means for C++ support?

LMGTFY: https://github.com/Homebrew/homebrew-science/issues/1888

[jakirkham]

Not exactly clear there as it seems to be speculation by others. Though this response is pretty clear.

TL;DR One can build HDF5 with C++ and Fortran wrappers and threading support enabled. However, only the C API will be thread safe not the C++ or Fortran APIs.

Edit: Adding in this quote from one of the HDF5 developers from that post.

I wouldn't consider it safe for a distribution to ship with both options enabled, since it requires an application developer to put wrappers (or some other threadsafety mechanism) around the HDF5 C++ objects before the C++ API can be used in a threaded application.

[jakirkham]

Also note that the Homebrew developers reverted --enable-unsupported because it broke Fortran as noted in issue ( https://github.com/Homebrew/homebrew-science/issues/3054 ). See commit ( https://github.com/Homebrew/homebrew-science/commit/936cca5664c71277184eaec01f5bf315ab3e5527 ) for the reversion.

[jakirkham]

Maybe a solution would be to make thread support a feature and add a corresponding metapackage to track that. This way we could build HDF5 without the unsafe APIs (Fortran and C++) when threading is enabled and build Fortran and C++ when threading is disabled. Would this work for you @marqh?

[marqh]

Hi @jakirkham @ocefpaf

my current requirement is for a C API HDF5 library which is thread safe. I do not have a requirement for C++ or Fortran support for conda-forge provided packages

if a way could be agreed upon to meet this requirement then I am happy to contribute to its delivery

thank you
mark

[ocefpaf]

. I do not have a requirement for C++ or Fortran support for conda-forge provided packages

@astrofrog can you perform some test on the Fortran side to see if this PR is OK. I do have a colleague that uses the package exactly like this PR with both thread safe and Fortran without any issues. (On Linux... Maybe the homebrew problem is a OS X thing.)

[jakirkham]

Hi @qkoziol, was reading your statement about thread safety in HDF5 several years back. Am curious to know if enabling this will still cause issues for C++ and Fortran or if things have changed. Can you please advise us on whether this is an ok way to build HDF5 or not? Will this cause issues for applications using the C++ or Fortran API that do not necessarily lock before calling the API? Thanks.

[qkoziol]

I'm fairly confident that you won't have any problems with C++ and threadsafe mode in HDF5, but the situation is still the same as it was in the past: unsupported.

[jakirkham]

Thanks @qkoziol.

[jakirkham]

@ukoethe, do you think this will cause any problems with VIGRA's HDF5 support?

[jakirkham]

cc @stuarteberg

[ukoethe]

@jakirkham Why do you suspect that this might interfere with VIGRA's build process?

[jakirkham]

It's not a concern with VIGRA's build process. It is a concern w.r.t. threading. In particular would enabling thread support in HDF5 have any unintended consequences with VIGRA? As I'm not really familiar with what goes on in those code paths, I was hoping you would know more.

[ukoethe]

Class ChunkedArrayHDF5 explicitly synchronizes concurrent access to the dataset it references. However, multiple chunked array objects referring to different datasets in the same HDF5 file are not jointly synchronized. Class HDF5File and the functionality around it (e.g. random forest I/O) are not synchronized.

I suppose that thread-safe HDF5 adds synchronization of its own. It shouldn't interfere with VIGRA per se (unless it is incompatible with std::thread), but I can imagine that naive use of VIGRA's primitives in a multi-threaded programm might lead to deadlock.

What else do you think could go wrong?

[jakirkham]

I suppose that thread-safe HDF5 adds synchronization of its own.

IIUC there is no synchronization around the Fortran or C++ APIs. Though I could be missing something.

ref: https://lists.hdfgroup.org/pipermail/hdf-forum_lists.hdfgroup.org/2009-June/000135.html

[ukoethe]

VIGRA builds its own C++ wrappers solely from the C-API, so it might actually work.

[qkoziol]

If you have thread-safe C++ wrappers built around the HDF5 C API, you will be fine.

…

On Mar 1, 2017, at 9:52 AM, Ullrich Koethe ***@***.***> wrote: VIGRA builds its own C++ wrappers solely from the C-API, so it might actually work. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFAhwpOQxgTV4VxrtaGIc6OI19Z2qlfMks5rhbB7gaJpZM4MEbUr>.

[ukoethe]

As I said, VIGRA's HDF5File and HDF5Handle are not thread-safe, but there is also no reason to share objects of these classes among threads -- I'm not aware of any use-cases where thread-local storage wouldn't work. The only class that may need attention is HDF5HandleShared, which should be reimplemented as a wrapper around std::shared_ptr and the corresponding atomics to become thread-safe.

[marqh]

it is sounding to me like there is a fair amount of support for providing the HDF library built in this way, threadsafe and unsupported

@jakirkham do you think this represents a stable enough approach to be adopted? It would make a significant impact for us if it could

It seems clearner to me than trying to manage both non-threadsafe and threadsafe build varients, via metapackages or some other mechanism.

[marqh]

Hello @jakirkham

please may we have an update from you on your thoughts on this work?

Do you still have fundamental concerns about this approach?
Do you have alternative proposals which could be implemented?

thank you
mark

[jakirkham]

I remain uneasy about integrating this into the default HDF5 build. It is too difficult to reason about all the effects of this on the many libraries in conda-forge that rely on HDF5. Also I'm not sure how this impacts other things like MPI. ( #51 ) Would much prefer to see this as some separate feature, variant, or similar that a user can optionally enable, but is by default disabled.

[minrk]

From the linked HDF5 list comment, this seems okay to me, because:

1. threadsafety only applies to the C API
2. C++ / FORTRAN APIs are unaffected

The argument for --enable-unsupported seems to be to prevent the confusion of a user typing:

./configure --enable-threadsafe --enable-cxx

and expecting threadsafe C++, which they won't get. It doesn't appear that C++ or FORTRAN is harmed by the presence of threadsafe C, just that the flags don't communicate that threadsafe only applies to the C API.

If that's correct, the only argument I see against threadsafety would be if there's a significant performance cost in the single-threaded case.

[ocefpaf]

It seems we have more people pro than con for this PR. Merging this then. If we find any problem with it we can pull the binaries. (But I really doubt it. I've been using this version of HDF5 for almost an year now.)

[cpelley]

Thanks everyone for this, helps us out a lot :)
ping @marqh

[marqh]

@ocefpaf @jakirkham

this represents a large step forward for us. This has been the major blocker for my community in adopting conda-forge

I have created #62 requesting that I be adopted as a maintainer, I hope this is reasonable request and that I can help with the ongoing maintenance of this package

[ocefpaf]

This has been the major blocker for my community in adopting conda-forge

I am sorry for that. I've been using this modification for so long in our internal build that I forgot about the conda-forge community. I apologize for not merging this earlier.