search

evaleev / libint

Libint: high-performance library for computing Gaussian integrals in quantum mechanics
Other
218 stars 97 forks source link

new cmake harness #205

Open evaleev opened 3 years ago

evaleev commented 3 years ago

cmake build for compiler and library. supersedes #148 .

status board based on https://github.com/evaleev/libint/pull/148#issuecomment-754412335

Major features

TODOs

Won't fix in this PR

evaleev commented 3 years ago

@loriab since #200 introduces order-3+ derivatives of 2-body ints we need to generalize the component naming ... is this still relevant?

evaleev commented 2 years ago

@loriab since #200 introduces order-3+ derivatives of 2-body ints we need to generalize the component naming ... is this still relevant?

@loriab I'm generalizing component handling, please let me know if this is still relevant for psi4

loriab commented 2 years ago

Some of the primary configuration control names appear in several places roughly itemized below. I don't imagine the code proper, (4), is changing. But (2) and (3) have gone from ERI3 to TWOBODY3. Would you like me to roll that out to (1), thereby changing mostly top-level CMakeLists.txt and the rhs of config.h.cmake.in?

  1. top-level CMakeLists.txt options like ENABLE_ERI3 and WITH_ERI3_MAX_AM https://github.com/evaleev/libint/pull/205/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR157
  2. options validation switched to ENABLE_TWOBODY3 in int_am.cmake https://github.com/evaleev/libint/pull/205/files#diff-61cbcd265d3286be79ccd7726988d6f08c7168aa8585ca63a752f4aa9128c191R196
  3. cmake components detection also uses new form e.g., twobody_c3_d0_l6 https://github.com/evaleev/libint/pull/205/files#diff-18b883fee9346f934717ce3fb8c5c56e3281b31ebfd14dcc7fec975d5a0c35bbR80
  4. handing options to c++ code in config.h. "lhs" in expressions like #define ERI3_MAX_AM @ERI3_MAX_AM@ The "rhs" are defined by 2. above in int_am.cmake . https://github.com/evaleev/libint/pull/205/files#diff-33850742674d410a506959be34a11003a5f6b4a82b365bf21eafcb22aaa9a4b3R122-R126
loriab commented 2 years ago

progress update: I'm reworking int_am.cmake to incorporate your new ints class names, arbitrary max deriv, and better opt_am defaulting from max_am. I'll post a separate PR to this branch when I've got it (the one file) operational. Also drafting INSTALL.md guide as I go.

loriab commented 2 years ago

I'm concerned about how eigen is handled in the library export. The local libint2::libint-eigen3 target is exported at library build time https://github.com/evaleev/libint/pull/205/files#diff-8e69ff48dd421c27038dafc327ded859e9be46b2e1aecec070d8b263a7b03d02R142 so the build-computer path shows up in the libint2-targets-*cmake files (along with other full paths to the libint2::cxx target itself)

# Create imported target Libint2::libint-Eigen3
add_library(Libint2::libint-Eigen3 INTERFACE IMPORTED)

set_target_properties(Libint2::libint-Eigen3 PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "/psi/toolchainconda/envs/py39d/include/eigen3"
)

# Create imported target Libint2::int2
add_library(Libint2::int2 STATIC IMPORTED)

set_target_properties(Libint2::int2 PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_11"
  INTERFACE_INCLUDE_DIRECTORIES "/psi/gits/libint2-efv/build01/library-install-stage/include"
)

But that library build computer isn't necessarily the computer on which the library is used, and the eigen might be a different distribution. Below are some snippets from the libint2-targets-*cmake from the original #148 without full paths and with generic eigen target. (I guess the eigen target should be detected by find_dependency(Eigen3) in libint2-config.cmake? I admittedly didn't have that working. One could make something like https://github.com/SebWouters/CheMPS2/blob/master/CMake/FindTargetHDF5.cmake to detect a home-grown eigen target since it looks like you don't want to rely upon eigen installed with config files alongside.)

# Create imported target Libint2::cxx
add_library(Libint2::cxx SHARED IMPORTED)

set_target_properties(Libint2::cxx PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Eigen3::Eigen"
)

set_target_properties(Libint2::cxx PROPERTIES
  IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libint2.so.2"
  IMPORTED_SONAME_RELEASE "libint2.so.2"

Am I correct in finding this a concern, or am I missing something?

evaleev commented 2 years ago

@loriab I am trying to fix the PR so that CI passes ... note that I partially reverted 76b6605 because the amount of changes needed to replace eri with twobody is fairly massive ... I actually implemented them, CLion ate them, and I am not going to waste the hours again :(

re prepending LIBINT_ to all variables: the goal was to "namespace" all Libint-specific variables (some already are, but the legacy stuff like ERI_MAX_AM etc at not namespaces) just to make subproject builds a bit saner. But again much code depends on the current naming convention for the variables, so perhaps postponing this until it's actually needed is the way to go.

will have a look at the Eigen export concerns later today.

evaleev commented 2 years ago

I'm concerned about how eigen is handled in the library export.

so am I ... this is the least fragile I could come up with. For a "header-only" library it's sure difficult to consume it in cmake. Issues:

I am not saying that the current approach cannot/should not be improved, but basically we need to support non-cmake-configured eigen3, handle the corner case of the per-user registry with its build tree, etc.

set_target_properties(Libint2::int2 PROPERTIES INTERFACE_COMPILE_FEATURES "cxx_std_11" INTERFACE_INCLUDE_DIRECTORIES "/psi/gits/libint2-efv/build01/library-install-stage/include" )

is this the build-tree targets file? If yes then it's correct. The install-tree targets file should not contain refs to the build tree (see https://github.com/evaleev/libint/blob/e886334745e6a7e01f489e2912a0b3875a7b2dbc/src/lib/libint/CMakeLists.txt.export#L222 )

evaleev commented 2 years ago

@loriab the CI jobs currently fail due to some branches in int_am.cmake not having been updated to the new component names: https://github.com/evaleev/libint/blob/e886334745e6a7e01f489e2912a0b3875a7b2dbc/cmake/modules/int_am.cmake#L129

loriab commented 2 years ago

@loriab I am trying to fix the PR so that CI passes ... note that I partially reverted 76b6605 because the amount of changes needed to replace eri with twobody is fairly massive ... I actually implemented them, CLion ate them, and I am not going to waste the hours again :(

sounds good, will stick with ERI3 and eri_c3_d1_l4, etc.

re prepending LIBINT_ to all variables: the goal was to "namespace" all Libint-specific variables (some already are, but the legacy stuff like ERI_MAX_AM etc at not namespaces) just to make subproject builds a bit saner. But again much code depends on the current naming convention for the variables, so perhaps postponing this until it's actually needed is the way to go.

thanks, saving for future pass sounds good to me.

I see you bumped CMake min to 3.16, hooray! Any objections to me bumping to 3.18 (released July 2020) to simplify int_am.cmake?

@loriab the CI jobs currently fail due to some branches in int_am.cmake not having been updated to the new component names

The new component names are healed in #233 . I'll get that rebased and moved back to ERI3 and see where the next CI error happens :-)

is this the build-tree targets file? If yes then it's correct. The install-tree targets file should not contain refs to the build tree (see

Right, that was the build-tree targets file, as I had some paths mix-up. But the install-tree file looks the same with full paths.

I am not saying that the current approach cannot/should not be improved, but basically we need to support non-cmake-configured eigen3, handle the corner case of the per-user registry with its build tree, etc.

Ok, I think I have a cmake pattern for a dependency that's not necessarily detected as target so can fulfill those requirements.

The parts that are confusing me are:

  • "requires remembering how it was discovered, caching the path, etc."

But the library building computer (e.g., Fedora package builder) and the consumer of libint2-config.cmake (grad student building mpqc) won't be using the same eigen location, so I'm not seeing why remembering/caching is needed. Is there an eigen version constraint that needs transmitting to the consumer?

  • "There is no guarantee that the C++ interface with be header-only in the future, so it may depend on the particular Eigen ABI."

To clarify, this means that the Libint C++ interface mayn't be purely header-only in future, so the libint2-library-generation eigen version might need to match the eigen version present when, say, psi4 links against a pre-built libint2? This sounds do-able. And actually sounds rather like the Boost usage?

For a "header-only" library it's sure difficult to consume it in cmake.

I've found this, too, via pybind11 and HelPME (an Andy Simmonett project). All the header-only distribution and inclusion flexibility to the user makes the CMake handling a pain.

Thanks for all the comments and commits!

evaleev commented 2 years ago

I see you bumped CMake min to 3.16, hooray! Any objections to me bumping to 3.18 (released July 2020) to simplify int_am.cmake?

I suggest we keep it at 3.16 ... that's the default in ubuntu 20.04 which is the most common ubuntu out there.

The new component names are healed in #233

https://github.com/evaleev/libint/pull/233 . I'll get that rebased and moved back to ERI3 and see where the next CI error happens :-)

is this the build-tree targets file? If yes then it's correct. The install-tree targets file should not contain refs to the build tree (see

Right, that was the build-tree targets file, as I had some paths mix-up. But the install-tree file looks the same with full paths.

So the install-tree's libint2-config has a build path in it?

The parts that are confusing me are:

  • "requires remembering how it was discovered, caching the path, etc."

But the library building computer (e.g., Fedora package builder) and the consumer of libint2-config.cmake (grad student building mpqc) won't be using the same eigen location, so I'm not seeing why remembering/caching is needed. Is there an eigen version constraint that needs transmitting to the consumer?

  • "There is no guarantee that the C++ interface with be header-only in the future, so it may depend on the particular Eigen ABI."

To clarify, this means that the Libint C++ interface mayn't be purely header-only in future, so the libint2-library-generation eigen version might need to match the eigen version present when, say, psi4 links against a pre-built libint2? This sounds do-able. And actually sounds rather like the Boost usage?

Yes, basically Libint's C++ interface will be only optionally header only. The actual way I use Libint in MPQC is I instantiate the Engine code in a single .cpp file to avoid the compile time impact of engine.impl.h ... if I instantiate Engine in a libint target that target will depend on Eigen ABI (not on Boost though ... Boost is only used for preprocessing) which means we better make sure we "link" against same Eigen as we discovered in Libint ... so we need to cache locations of its config file and use it as a hint in libint2-config file, etc. ... Eigen's configuration affects its ABI (e.g. can force it to dispatch to BLAS/LAPACK, etc.) so we MUST make sure the target we used to build Libint is the exact same one (not only version, mind you, but the actual instance of that target). Right now the default is to keep C++ interface as header only, but it's already an overly-expensive (compile-time-wise) way to use it, so it probably needs to change, or at least there should be an optional component that instantiates the Engine.

I've found this, too, via pybind11 and HelPME (an Andy Simmonett project). All the header-only distribution and inclusion flexibility to the user makes the CMake handling a pain.

Header-only libraries are fine for newbies/one-off uses. But too brittle without an accompanying cmake/meson exported targets.

-- web: valeyev.net

loriab commented 2 years ago

fyi that the gha file in this branch is using commas and _LIST options that I don't think will process as expected with cmake or int_am as originally designed. The changes below configure as expected with my revised int_am.

https://github.com/evaleev/libint/pull/233/files#diff-cdd48abbd3eb8d1c54077449fc74a8de1f29805d2be5d8e5232b7aab76ea7a6f

evaleev commented 2 years ago

fyi that the gha file in this branch is using commas and _LIST options that I don't think will process as expected with cmake or int_am as originally designed. The changes below configure as expected with my revised int_am.

https://github.com/evaleev/libint/pull/233/files#diff-cdd48abbd3eb8d1c54077449fc74a8de1f29805d2be5d8e5232b7aab76ea7a6f

that's fine, the natural cmake separators are fine

loriab commented 2 years ago

It looks like parts of library test 10 may be sensitive to SHGAUSS. All tests pass with standard, but -DLIBINT2_SHGAUSS_ORDERING=gaussian leads to the below:

10:  16     -76.003354058439   1.184157242911e-15   1.418467295368e-12    0.03415
10: compute_2body_fock:precision = 2.22044604925031e-16
10: will set Engine::precision as low as = 1.63035233462786e-07
10: time for integrals = 0.0272398820000002
10: # of integrals = 411041
10:  17     -76.003354058439   1.522487883743e-15   9.885192866120e-13    0.03339
10: HF energy check: passed
10: electric dipole moment check: passed
10: electric quadrupole moment check: passed
10: spherical electric dipole moment check: failed
10: reference: [0.04560184159274555, -0.105312942076475, 0.1316411775955935] 
10: actual: ** sph edipole = -0.105312942035006 0.131641177543759 0.0456018415747894 
10: 
10: spherical electric quadrupole moment check: failed
10: reference: [0.0061560863769835, -0.3191024539954775, 0.5904181715318579, 0.2881006495120795, 0.12874628868708882] 
10: actual: ** sph equadrupole = 0.590418171501538 0.288100649497834 -0.319102453978188 0.12874628867967 0.00615608637620421 
10: 
10: 1-body force check: passed
10: Pulay force check: passed
10: compute_2body_fock:precision = 2.22044604925031e-16
10: will set Engine::precision as low as = 2.21669529173275e-16
10: time for integrals = 0.127778161
10: # of integrals = 4932492
10: 2-body force check: passed
10: nuclear repulsion force check: passed
10: HF force check: passed
loriab commented 2 years ago

I was experimenting with the compiled cxx interface. Switching the library tests from Libint2::cxx (ho) to Libint2::int2-cxx (compiled) leads to them being linked (via ldd) to int2 but not int2-cxx. Trying to force the compiled interface via

+    target_compile_definitions(
+      hf++-libint2
+      PUBLIC
+        LIBINT2_DOES_NOT_INLINE_ENGINE=1
+        __COMPILING_LIBINT2=1
+      )

leads to build-time errors like

test 9
      Start  9: libint2/hf++/build

9: Test command: /psi/toolchainconda/envs/basenol2/bin/cmake "--build" "/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build" "--target" "hf++-libint2"
9: Test timeout computed to be: 1500
9: [1/2] Building CXX object tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o
9: FAILED: tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o 
9: /psi/toolchainconda/envs/basenol2/bin/x86_64-conda-linux-gnu-c++ -DBOOST_ALL_NO_LIB -DLIBINT2_DOES_NOT_INLINE_ENGINE=1 -DSRCDATADIR=\"/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/lib/basis\" -D__COMPILING_LIBINT2=1 -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/src -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build/include -I/psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library-build/include/libint2 -isystem /psi/toolchainconda/envs/singleeigen/include/eigen3 -isystem /psi/toolchainconda/envs/singleboost/include -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /psi/toolchainconda/envs/basenol2/include -march=native -O3 -DNDEBUG -MD -MT tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o -MF tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o.d -o tests/CMakeFiles/hf++-libint2.dir/hartree-fock/hartree-fock++.cc.o -c /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc
9: In file included from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7.h:24,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/statics_definition.h:26,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc:53:
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:66: error: 'cheb_table_nintervals' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                  ^~~~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:90: error: 'cheb_table_mmax' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                                          ^~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/boys_cheb7_v2.h:34:110: error: 'interpolation_order' was not declared in this scope
9:    34 | template<> double libint2::FmEval_Chebyshev7<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+1)*(interpolation_order+1)]=
9:       |                                                                                                              ^~~~~~~~~~~~~~~~~~~
9: In file included from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb.h:24,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/statics_definition.h:27,
9:                  from /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/tests/hartree-fock/hartree-fock++.cc:53:
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:60: error: 'cheb_table_nintervals' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                            ^~~~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:84: error: 'cheb_table_mmax' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                    ^~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:104: error: 'interpolation_order' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                                        ^~~~~~~~~~~~~~~~~~~
9: /psi/gits/libint2-efv/build-27/src/lib/libint/library-prefix/src/library/include/libint2/tenno_cheb15.h:15:128: error: 'interpolation_order' was not declared in this scope
9:    15 | template<> double libint2::TennoGmEval<double>::cheb_table[cheb_table_nintervals][(cheb_table_mmax+2)*(interpolation_order+1)*(interpolation_order+1)]=
9:       |                                                                                                                                ^~~~~~~~~~~~~~~~~~~
9: ninja: build stopped: subcommand failed.
 9/14 Test  #9: libint2/hf++/build ...............***Failed   12.86 sec
test 10
      Start 10: libint2/hf++/run
Failed test dependencies: libint2/hf++/build
10/14 Test #10: libint2/hf++/run .................***Not Run   0.00 sec

Most likely, I'm just using the compiled interface wrong. But in case this rings a bell about more header manipulations needed, I thought I'd post it.