Add OpenMP kernels

[mcbarton]

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.

• Bug fix
• New feature
• Added/removed dependencies
• Required documentation updates

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.56%. Comparing base (4d2e571) to head (1dccd67).

Files with missing lines	Patch %	Lines
src/xinterpreter.cpp	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   80.28%   80.56%   +0.27%     
==========================================
  Files          21       21              
  Lines         847      854       +7     
  Branches       78       80       +2     
==========================================
+ Hits          680      688       +8     
+ Misses        167      166       -1     

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	89.24% <92.30%> (+1.16%)	⬆️

Files with missing lines	Coverage Δ
src/xinterpreter.cpp	89.24% <92.30%> (+1.16%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcbarton]

Things done so far

• Add c++ + openmp kernels (includes kernel definition json files added, and cmake change)
• Updated documentation so that LD_LIBRARY_PATH is defined
• Add example c++ + openmp notebooks which have been tested locally to work (taken from xeus-clang-repl)

List of things still to do

• Add c + openmp kernels
• Update ci so LD_LIBRARY_PATH is defined and openmp kernels can be tested
• Add some tests for the openmp kernels

[vgvassilev]

We should probably have a openmp specific ci without having to rely on the ld_library_path..

[mcbarton]

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.

[vgvassilev]

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.

Okay, let's see if that's compatible with the vision of @SylvainCorlay which he expressed recently.

[mcbarton]

@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.

[Vipul-Cariappa]

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?

[mcbarton]

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.

[vgvassilev]

@mcbarton, can you squash all into a single commit?

[mcbarton]

@mcbarton, can you squash all into a single commit?

@vgvassilev I have squashed all commits into a single commit

[vgvassilev]

Quick question : Can we just start with 1 OMP kernel instead of starting with 6 ?

Why maintain 6 when we are planning to refactor into 1 paramterized kernel spec !

Looks like a permutation and combination of language * versions * omp to me.

Even if there's time before the parameterized kernel spec lands, I think its just too much to maintain :\

So if we land a cuda kernel as was being discussed recently on discord, we again land 6 more kernel ?

Can we add one kernel per language’s highest supported standard version then?

[mcbarton]

Quick question : Can we just start with 1 OMP kernel instead of starting with 6 ?
Why maintain 6 when we are planning to refactor into 1 paramterized kernel spec !
Looks like a permutation and combination of language * versions * omp to me.
Even if there's time before the parameterized kernel spec lands, I think its just too much to maintain :
So if we land a cuda kernel as was being discussed recently on discord, we again land 6 more kernel ?

Can we add one kernel per language’s highest supported standard version then?

@vgvassilev Done. There is now just a single openmp kernel for each language. A c++23 + openmp kernel, and a c23 + openmp kernel.

[anutosh491]

Can we add one kernel per language’s highest supported standard version then?

Hopefully that makes sense !?

Some standard C/C++ kernels + 1 kernel per speciliazed case sounds just good enough to maintain right ?

[vgvassilev]

Can we add one kernel per language’s highest supported standard version then?

Hopefully that makes sense !?

Some standard C/C++ kernels + 1 kernel per speciliazed case sounds just good enough to maintain right ?

To be honest there is no maintenance cost for these things so far but let's start with what you suggest.

[mcbarton]

Can we add one kernel per language’s highest supported standard version then?

Hopefully that makes sense !?
Some standard C/C++ kernels + 1 kernel per speciliazed case sounds just good enough to maintain right ?

To be honest there is no maintenance cost for these things so far but let's start with what you suggest.

I think I have implemented what has been suggested with only 1 c and 1 c++ openmp kernel. I may need further explaination if I have not.

[vgvassilev]

This looks like getting very close to a merge. Can we have only one hello world notebook in tree and the rest to move to https://github.com/compiler-research/live-cpp-tutorials

We will need to figure out how to run these as part of the ci.

[mcbarton]

This looks like getting very close to a merge. Can we have only one hello world notebook in tree and the rest to move to https://github.com/compiler-research/live-cpp-tutorials

We will need to figure out how to run these as part of the ci.

I will move these notebooks during my lunch break.

I already know how to run the notebooks as part of the ci. This PR we talked about the other day on Discord runs the notebooks for Jupyter Lite as part of the ci #415 . Once that is in, I will work on modifying the script so it can run notebooks for the native case too.

[mcbarton]

I've now reduced to just the hello world notebook, and opened up the PR moving the other notebooks here compiler-research/live-cpp-tutorials#1