-
Notifications
You must be signed in to change notification settings - Fork 36
Add OpenMP kernels #389
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
base: main
Are you sure you want to change the base?
Add OpenMP kernels #389
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
=======================================
Coverage 81.78% 81.78%
=======================================
Files 20 20
Lines 950 950
Branches 87 87
=======================================
Hits 777 777
Misses 173 173 🚀 New features to boost your workflow:
|
e4af2d6
to
3824a2f
Compare
7dda968
to
cc7f3c2
Compare
Things done so far
List of things still to do
|
6c4d0f4
to
dbe5005
Compare
We should probably have a openmp specific ci without having to rely on the ld_library_path.. |
@vgvassilev Having an openmp specific ci will not avoid the need for ld_library_path. To avoid ld_library_path you would need to rework the cmake and the kernelspec (which makes reference to this variable) , and the example notebooks would need to know the path where you stored the openmp library. Using ld_library_path like xeus-clang-repl did simplifies this PR. |
45a4ee5
to
571023c
Compare
Okay, let's see if that's compatible with the vision of @SylvainCorlay which he expressed recently. |
1feccb1
to
1b1b9cd
Compare
#include <omp.h> | ||
#include <iostream> | ||
#include <clang/Interpreter/CppInterOp.h> | ||
Cpp::LoadLibrary("libomp"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cpp::LoadLibrary("libomp"); works in the notebooks, but doesn't appear to be working in the python kernel tests. I believe this because the tests fail with the same error that I get if I remove this line from the notebook (see https://github.com/compiler-research/xeus-cpp/actions/runs/18385849945/job/52383968040?pr=389#step:13:1015 for error). @vgvassilev @anutosh491 Any idea what may be going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only guess is that the kernelspecs have the flag -fopenmp=libomp , but the python output has the fopenmp flag without the =libomp (see https://github.com/compiler-research/xeus-cpp/actions/runs/18385849945/job/52383968040?pr=389#step:13:922 ). I don't know where to change it so that it has -fopenmp=libomp too though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call the Cpp::LoadLibrary("libomp")
in a separate declare operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if you was suggesting splitting this code into 2 executions, one which loads the library, and one which executes the openmp code, but that has worked, and we now have a passing test. Its a simple OpenMP test, but hopefully it will suffice.
5d1ad18
to
5a2630d
Compare
@vgvassilev @alexander-penev @Vipul-Cariappa @anutosh491 @SylvainCorlay I now consider this PR ready for review. It adds c and c++ kernels that have openmp. It includes example notebooks for the c++ + openmp kernels taken from xeus-clang-repl which were able to run when I tried locally. It also has one simple openmp test for the jupyter kernel test tests. I wasn't exactly sure what would suffice as an automatic test, but the test clearly demonstrates the kernel is executing OpenMP code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to these changes; The user after selecting a omp kernel, still needs to include CppInterOp.h
and call Cpp::LoadLibrary(...)
. The user should not be expected to know the internals and do this.
When we launch a kernel that enables OpenMP, it should automatically load all necessary stuff. You need to parse the compiler arguments, check if it contains -fopenmp
, if yes, call LoadLibrary
.
Also, can you share the contents of kernel.json
files that gets installed.
Stretch goal (may not be possible): Can we get it to work, without modifying the LD_LIBRARY_PATH
env var?
I think I have an idea of how to avoiding needing ld library path. My new method should work on the Windows once we have that platform working too. I just need to test it out, and plan to test it out sometime in the next few days. I'm not so convinced that making the user have to include CppInterOp.h, and calling cpp::LoadLibrary(...) is as big deal as you make out to be. It would also let the user know to use other (non openmp) shared libraries in their notebooks. The example notebooks are there for a reason. I kept this PR the way xeus-clang-repl did openmp kernels. Despite this I will look into working out to have the kernel automatically do this if it notices -fopenmp in the kernel arguments. |
2dac7b4
to
a41ed55
Compare
# Used to configure OpenMP kernels such that Cpp::loadlibrary knows where OpenMP library is | ||
if (NOT WIN32) | ||
list(GET OpenMP_CXX_LIBRARIES 0 FirstOpenMPLibrary) | ||
get_filename_component(OpenMPLibraryDir "${FirstOpenMPLibrary}" DIRECTORY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the path to the openmp library path from cmake is how I managed to avoid having the user manual set ld_library_path. The kernels are then configured using this variable. @vgvassilev @Vipul-Cariappa Hopefully this an acceptable alternative. LD_LIBRARY_PATH is still mentioned in the kernelspec, but it no longer comes from the user setting variables. Its needed otherwise CppInterOp doesn't know where the openmp library is it needs to load.
I couldn't get xeus-cpp to parse the arguments of the kernelspec and it finds -fopenmp
, to automatically include CppInterOp.h and call Cpp::LoadLibrary(...)
on the openmp library. Can I open this as an issue instead of fixing it as part of this PR?
…p to native conda environment
Add export LD_LIBRARY_PATH="$CONDA_PREFIX/lib/:$LD_LIBRARY_PATH" to ci
a41ed55
to
7380443
Compare
Description
Please include a summary of changes, motivation and context for this PR.
Fixes # (issue)
Type of change
Please tick all options which are relevant.