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

CMake: add HDF5 support #1048

Merged
merged 8 commits into from
Jun 24, 2020
Merged

Conversation

mic84
Copy link
Contributor

@mic84 mic84 commented Jun 24, 2020

No description provided.

@mic84 mic84 requested a review from atmyers June 24, 2020 00:24
@mic84 mic84 marked this pull request as draft June 24, 2020 00:24
@mic84 mic84 requested a review from ax3l June 24, 2020 00:24
@mic84
Copy link
Contributor Author

mic84 commented Jun 24, 2020

@eschnett can please try this out?

@eschnett
Copy link
Contributor

This fails, for reasons probably unrelated to your work, but due to other changes. I receive this error message:

COMPILING CarpetX/CarpetX/src/io_silo.cxx
/__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/io_silo.cxx: In function 'bool CarpetX::operator==(const CarpetX::mesh_props_t&, const CarpetX::mesh_props_t&)':
/__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/io_silo.cxx:44:25: error: ambiguous overload for 'operator==' (operand types are 'std::tuple<amrex::IntVect>' and 'std::tuple<amrex::IntVect>')
   44 |     return p.to_tuple() == q.to_tuple();
      |            ~~~~~~~~~~~~ ^~ ~~~~~~~~~~~~
      |                      |               |
      |                      tuple<[...]>    tuple<[...]>
In file included from /usr/include/c++/9/functional:54,
                 from /usr/include/c++/9/pstl/glue_algorithm_defs.h:13,
                 from /usr/include/c++/9/algorithm:71,
                 from /__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/loop.hxx:8,
                 from /__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/driver.hxx:4,
                 from /__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/io_silo.cxx:3:
/usr/include/c++/9/tuple:1419:5: note: candidate: 'constexpr bool std::operator==(const std::tuple<_Tps ...>&, const std::tuple<_Elements ...>&) [with _TElements = {amrex::IntVect}; _UElements = {amrex::IntVect}]'
 1419 |     operator==(const tuple<_TElements...>& __t,
      |     ^~~~~~~~
In file included from /usr/local/include/AMReX_Gpu.H:29,
                 from /usr/local/include/AMReX_Reduce.H:5,
                 from /usr/local/include/AMReX_BaseFab.H:32,
                 from /usr/local/include/AMReX_TagBox.H:10,
                 from /usr/local/include/AMReX_AmrMesh.H:12,
                 from /usr/local/include/AMReX_AmrCore.H:8,
                 from /__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/driver.hxx:12,
                 from /__w/1/s/Cactus/arrangements/CarpetX/CarpetX/src/io_silo.cxx:3:
/usr/local/include/AMReX_GpuAllocators.H:141:5: note: candidate: 'bool amrex::operator==(const Allocator<T>&, const Allocator<U>&) [with Allocator = std::tuple; T = amrex::IntVect; U = amrex::IntVect]'
  141 |     operator==(Allocator<T> const&, Allocator<U> const&) noexcept
      |     ^~~~~~~~

It seems that AMReX provides an operator== that is too generic, and which overlaps with other definitions (AMReX_GpuAllocators.H:141):

    template <template <typename> class Allocator, class T, class U>
    bool
    operator==(Allocator<T> const&, Allocator<U> const&) noexcept
    {
        return true;
    }

I think this operator accepts essentially any templated class, and hence leads to ambiguities in many cases.

@WeiqunZhang
Copy link
Member

I have seen operator overloading ambiguity (on a different type in a different code) with gcc-9.3.0 and c++17 or c++2a. gcc-9 with c++14 worked. gcc <9 and gcc 10 also worked. I believe it was a compiler bug in that case. Could try a different compiler or different standard if you are using C++17.

@WeiqunZhang
Copy link
Member

Sorry, I didn't read the error message carefully. I think you are right. It is our fault.

@eschnett
Copy link
Contributor

For the record, I'm using GCC 10 with C++ 17.

@WeiqunZhang
Copy link
Member

The operator is fixed in #1050, I believe.

@mic84
Copy link
Contributor Author

mic84 commented Jun 24, 2020

@eschnett can you try and let us know if it works?

@eschnett
Copy link
Contributor

Trying it... I see that AMReX requires at least HDF5 1.12. At the same time, our code is stuck at 1.10 for some compatibility reason. (Or maybe that's just Ubuntu 20.04?) I will check and/or update our code, but that might take some time.

Is this version requirement new? It wasn't there yesterday.

@atmyers
Copy link
Member

atmyers commented Jun 24, 2020

Let me double-check the minimum version required, it may be that we can relax this.

@atmyers
Copy link
Member

atmyers commented Jun 24, 2020

It seems that we could drop this to at least 1.10.4.

@eschnett
Copy link
Contributor

1.10.4 is what we currently use.

@mic84
Copy link
Contributor Author

mic84 commented Jun 24, 2020

I modified CMake to accept HDF5 >= 1.10.4. Please try again.

@eschnett
Copy link
Contributor

Another build error:

[ 46%] Building CXX object Src/CMakeFiles/amrex.dir/Base/AMReX_GpuControl.cpp.o
/cactus/src/amrex/Src/Base/AMReX_PlotFileUtil.cpp: In function 'void amrex::WriteMultiLevelPlotfileHDF5(const string&, int, const amrex::Vector<const amrex::MultiFab*>&, const amrex::Vector<std::__cxx11::basic_string<char> >&, const amrex::Vector<amrex::Geometry>&, amrex::Real, const amrex::Vector<int>&, const amrex::Vector<amrex::IntVect>&, const string&, const string&, const string&, const amrex::Vector<std::__cxx11::basic_string<char> >&)':
/cactus/src/amrex/Src/Base/AMReX_PlotFileUtil.cpp:765:9: error: 'H5Pset_fapl_mpio' was not declared in this scope; did you mean 'H5Pset_fapl_stdio'?
  765 |         H5Pset_fapl_mpio(fapl, MPI_COMM_SELF, MPI_INFO_NULL);
      |         ^~~~~~~~~~~~~~~~
      |         H5Pset_fapl_stdio

Do you require an HDF5 library that is configured with MPI? I'm currently using a stock serial Ubuntu HDF5 library.

@atmyers
Copy link
Member

atmyers commented Jun 24, 2020

We do currently require a parallel hdf5 library. @mic84 is there a way we can check and enforce that in the build system?

@mic84
Copy link
Contributor Author

mic84 commented Jun 24, 2020

@atmyers yes. Just submitted a commit to do just that. @eschnett can you try again one more time with both a parallel and serial hdf5 build? The serial build should now give you an error at configuration time, while the parallel build should be linked fine.

@eschnett
Copy link
Contributor

The next build error I encounter is in the Silo library (again – that's what constraining me to HDF5 1.10.x), which doesn't handle a parallel HDF5 library. When using parallel HDF5, then Silo requires HDF5 <=1.8.12. I'm going to have to make some choices here.

@mic84
Copy link
Contributor Author

mic84 commented Jun 24, 2020

Unfortunately, I am afraid that it cannot be fixed on the AMReX side. If you have no objections, I will merge this PR since I think that now everything is fine as far as AMReX is concerned.

@eschnett
Copy link
Contributor

eschnett commented Jun 24, 2020 via email

@atmyers atmyers marked this pull request as ready for review June 24, 2020 22:56
@atmyers atmyers changed the title [WIP] CMake: add HDF5 support CMake: add HDF5 support Jun 24, 2020
@atmyers atmyers merged commit 32d2e44 into AMReX-Codes:development Jun 24, 2020
@mic84 mic84 deleted the mr/cmake-hdf5-support branch June 24, 2020 23:02
#
if (ENABLE_HDF5)
set(HDF5_PREFER_PARALLEL TRUE)
find_package(HDF5 1.10.4 REQUIRED COMPONENTS CXX)
Copy link
Member

@ax3l ax3l Jun 26, 2020

Choose a reason for hiding this comment

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

From a very quick check, I think AMReX also implements the C API - also since the C++ bindings of HDF5 do not support parallel I/O (yet).
(This is not the same CMake component naming convention as in FindMPI here.)

This is what I do in openPMD-api (with a different target name):

find_package(HDF5 1.10.4 REQUIRED COMPONENTS C)

# ...
target_link_libraries(amrex PUBLIC ${HDF5_LIBRARIES})
target_include_directories(amrex SYSTEM PUBLIC ${HDF5_INCLUDE_DIRS})
target_compile_definitions(amrex PUBLIC ${HDF5_DEFINITIONS})

Note that the C++ bindings are also not built by default with HDF5.

dwillcox pushed a commit to dwillcox/amrex that referenced this pull request Oct 3, 2020
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.

5 participants