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

Use own Eigen and Eigen+MKL #19

Closed
wpoely86 opened this issue Feb 4, 2016 · 10 comments
Closed

Use own Eigen and Eigen+MKL #19

wpoely86 opened this issue Feb 4, 2016 · 10 comments

Comments

@wpoely86
Copy link
Contributor

wpoely86 commented Feb 4, 2016

  1. do not force the user to use the Eigen bundled with pcmsolver. There are many very good reason why I would want that pcmsolver uses the Eigen I have specified.

  2. Just setting EIGEN_USE_MKL_ALL to let Eigen use the MKL is not sufficient. You also need to link against it. Please add a FindMKL and add it to the link targets

@robertodr
Copy link
Member

Hi!

  1. I removed this possibility some time ago, since I saw no need for it. I can certainly reimplement it. Out of curiosity, which version of Eigen would you want to use the library with?
  2. This is on purpose, as specified here Finding MKL is not exactly trivial with CMake and I don't want to maintain a script that is never used. It should be fairly easy to peruse the MKL Link Line Advisor to find the correct link line for your MKL distribution and feed it to the setup.py script via the --extra-cxx-flags option. If that doesn't modify the link line to suit your needs, I'll have to modify the setup script.
    Out of curiosity (again) are you hitting some hotspots that you'd like to optimize away?

@wpoely86
Copy link
Contributor Author

wpoely86 commented Feb 4, 2016

Let me first explain the background: I'm installing a git version of PSI on our HPC system. Reproducibility and control of the build are key. We want and need to control and test all dependencies.

  1. Eigen is special that it's a headers only library but we still want to control which version it uses. If bugs are fixed, we don't want to be dependant on you to upgrade for example. We have version of Eigen that is test and should be used by all codes.

  2. There are plenty of FindMKL scripts out there. If you add it, you should add it properly. The define to use the MKL can also be passed by CMAKE_CXX_FLAGS. So, IMHO, either add it completely or not at all. I expect it to work when cmake -DENABLE_EIGEN_MKL=ON is run.

Furthermore, I see that the build directory is added to rpath. This is very wrong. Until I find time, I'm going to disable PCMSolver in PSI for our users.

@robertodr
Copy link
Member

Gotcha.

  1. I am working on it.
  2. Are you compiling within Psi4 or are you compiling the library on its own? In the former case, MKL is detected by Psi4 and at linking everything should work out. In the latter case, then I'd expect the user to know what he/she is doing... Anyway, I agree with you about "add it completely or not at all" and I'll choose the second option. This was a leftover from a refactoring of the CMake infrastructure.

Can you elaborate on the rpath bit?

@wpoely86
Copy link
Contributor Author

wpoely86 commented Feb 4, 2016

About the rpath:

[ 98%] Linking CXX executable ../../bin/run_pcm
cd /dev/shm/ward/PCMSolver/20160204/intel-2016a-Python-2.7.11/easybuild_obj/src/bin && /user/scratchdelcatty/gent/vsc403/vsc40307/EB/golett/software/CMake/3.4.1-intel-2016a/bin/cmake -E cmake_link_script CMakeFiles/run_pcm.dir/link.txt --verbose=1
/apps/gent/CO7/haswell-ib/software/icc/2016.1.150-GCC-4.9.3-2.25/compilers_and_libraries_2016.1.150/linux/bin/intel64/icpc   -O2 -xHost -ftz -fp-speculation=safe -fp-model source -O3 -DNDEBUG   -L/apps/gent/CO7/haswell-ib/software/icc/2016.1.150-GCC-4.9.3-2.25/lib/intel64 -L/apps/gent/CO7/haswell-ib/software/imkl/11.3.1.150-iimpi-8.1.5-GCC-4.9.3-2.25/lib -L/apps/gent/CO7/haswell-ib/software/imkl/11.3.1.150-iimpi-8.1.5-GCC-4.9.3-2.25/mkl/lib/intel64 -L/apps/gent/CO7/haswell-ib/software/imkl/11.3.1.150-iimpi-8.1.5-GCC-4.9.3-2.25/lib -L/apps/gent/CO7/haswell-ib/software/Python/2.7.11-intel-2016a/lib -L/apps/gent/CO7/haswell-ib/software/zlib/1.2.8-intel-2016a/lib -L/user/scratch/gent/vsc403/vsc40307/EB/golett/software/Boost/1.59.0-intel-2016a-Python-2.7.11/lib CMakeFiles/run_pcm.dir/run_pcm.cpp.o  -o ../../bin/run_pcm  -L/dev/shm/ward/PCMSolver/20160204/intel-2016a-Python-2.7.11/easybuild_obj/lib -rdynamic ../../lib/libpcm.so.1 -lz -Wl,-rpath,/dev/shm/ward/PCMSolver/20160204/intel-2016a-Python-2.7.11/easybuild_obj/lib 
make[2]: Leaving directory `/dev/shm/ward/PCMSolver/20160204/intel-2016a-Python-2.7.11/easybuild_obj'

In this case /dev/shm/ward is the build directory. As you can see in the end, -Wl,-rpath,/dev/shm/ward/PCMSolver/20160204/intel-2016a-Python-2.7.11/easybuild_obj/lib it's adding an rpath for the build directory which should not happen.

@robertodr
Copy link
Member

OK, I see. And that's BTW another leftover, thanks for pointing it out.
It seems you are compiling the library on its own. I assume you will pass the location of the built library to Psi4 via --pcmsolver-dir, correct?
I suggest you use these additional options to the setup.py script then:

--cmake-options='-DENABLE_LOGGER=OFF -DENABLE_TIMER=OFF -DBUILD_STANDALONE=OFF -DENABLE_FORTRAN_API=OFF -DENABLE_DOCS=OFF'

These are identical to the one for the build within Psi4. You will build the unit tests too.

@robertodr
Copy link
Member

You can give your own Eigen3 via the option --eigen=<EIGEN3_ROOT> to setup.py
Still looking at the rpath issue, it's trickier than I had thought.

@robertodr
Copy link
Member

Regarding the RPATH issue: run_pcm executable has an RPATH that points to both the install tree and the build tree. Other executables that are not installed (update_reference_files and unit_tests) have an RPATH that points to the build tree. libpcm.so.1 does not have an RPATH. Do you think it's a satisfactory solution?

@wpoely86
Copy link
Contributor Author

wpoely86 commented Feb 5, 2016

In my case, run_pcm has an rpath to the build directory. It's not really an issue for me as our build directory are always temporarily but in the general case, this could lead to very ugly problems: if you change stuff in your personal build directory, it could break an installed version of run_pcm etc.

I found another issue: in one of the files (tests/C_host/C_host-functions.h) there is some C99 code ( for(int i=0;i<... ) while the default mode for icc is still C89. Add the required C standard also to CMakeLists.txt? ( https://cmake.org/cmake/help/v3.4/manual/cmake-compile-features.7.html )

@robertodr
Copy link
Member

@wpoely86 I am keeping the solution outlined above for the RPATH. The standard for C is set to C99 for Intel, GNU and Clang compilers alike.
I also minted a stable v1.1.0 release, which is the one you should refer to if you're creating an easybuild "recipe".

P.S.: If you manage to get an easybuild "recipe" for Psi4 and addons, I'd be very grateful if you shared it.

@wpoely86
Copy link
Contributor Author

wpoely86 commented Feb 8, 2016

You shouldn't overwrite the CMAKE_C_FLAGS and friends flags. The user should be able to set them from the outside (like we do). The -std flag is not portable, hence the reason why CMake has it own infrastructure to add them.

I've got a working PSI4 easyconfig with PCMSolver. Only 4 tests fail: opt10, pubchem1, pubchem2, pywrap-opt-sowreap and sapt4. You can find it at easybuilders/easybuild-easyconfigs#2445 and it will be part of our next release. Feel free to give it a spin! Plugins, etc are working. It has been deployed on our systems.

@wpoely86 wpoely86 closed this as completed Feb 8, 2016
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

No branches or pull requests

2 participants