Skip to content

Add OpenMP kernels#389

Open
mcbarton wants to merge 7 commits intocompiler-research:mainfrom
mcbarton:Add-OpenMP-kernel
Open

Add OpenMP kernels#389
mcbarton wants to merge 7 commits intocompiler-research:mainfrom
mcbarton:Add-OpenMP-kernel

Conversation

@mcbarton
Copy link
Collaborator

@mcbarton mcbarton commented Oct 7, 2025

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
Copy link

codecov-commenter commented Oct 7, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.51%. Comparing base (3002a8e) to head (41df734).

Files with missing lines Patch % Lines
src/xinterpreter.cpp 92.30% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   81.25%   81.51%   +0.26%     
==========================================
  Files          21       21              
  Lines         864      871       +7     
  Branches       78       80       +2     
==========================================
+ Hits          702      710       +8     
+ Misses        162      161       -1     
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.61% <92.30%> (+0.99%) ⬆️
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.61% <92.30%> (+0.99%) ⬆️
🚀 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 mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from e4af2d6 to 3824a2f Compare October 9, 2025 07:23
@mcbarton
Copy link
Collaborator Author

mcbarton commented Oct 9, 2025

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
Copy link
Contributor

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

@mcbarton
Copy link
Collaborator Author

mcbarton commented Oct 9, 2025

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.

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from 45a4ee5 to 571023c Compare October 9, 2025 12:43
@vgvassilev
Copy link
Contributor

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.

#include <omp.h>
#include <iostream>
#include <clang/Interpreter/CppInterOp.h>
Cpp::LoadLibrary("libomp");
Copy link
Collaborator Author

@mcbarton mcbarton Oct 9, 2025

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

@mcbarton mcbarton Oct 10, 2025

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.

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 2 times, most recently from 5d1ad18 to 5a2630d Compare October 10, 2025 07:47
@mcbarton
Copy link
Collaborator Author

mcbarton commented Oct 10, 2025

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

Copy link
Collaborator

@Vipul-Cariappa Vipul-Cariappa left a 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?

@mcbarton
Copy link
Collaborator Author

mcbarton commented Oct 14, 2025

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.

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 3 times, most recently from 2dac7b4 to a41ed55 Compare October 15, 2025 12:34
Copy link
Collaborator

@Vipul-Cariappa Vipul-Cariappa left a comment

Choose a reason for hiding this comment

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

I too believe the CMake changes may not be required. At least the way it is implemented now. But we can track it in an issue, and fix it later.
Reasoning: OpenMP is not used by xeus-cpp itself (at least at build time), it is just an optional runtime dependency to the interpreter instance. Nowhere in xeus-cpp code we have #include <omp.h> ...

Otherwise, LGTM!

@SylvainCorlay
Copy link
Collaborator

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).

I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.

Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

@vgvassilev
Copy link
Contributor

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).

I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.

Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

Do we have an ETA for this?

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 4 times, most recently from a889c1b to f7552c9 Compare February 4, 2026 13:00
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"};
std::string resource_dir;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::string" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:23:

- #ifndef EMSCRIPTEN
+ #include <string>
+ #ifndef EMSCRIPTEN

if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) {
return s == "-resource-dir";}) == ExtraArgs.end()) {
std::string resource_dir = Cpp::DetectResourceDir();
resource_dir = Cpp::DetectResourceDir();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "CppImpl::DetectResourceDir" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:20:

- #include <algorithm>
+ #include <CppInterOp/CppInterOp.h>
+ #include <algorithm>

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 2 times, most recently from bcbe68f to 52721c2 Compare February 20, 2026 08:53
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"};
std::string resource_dir;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::string" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:23:

- #ifndef __EMSCRIPTEN__
+ #include <string>
+ #ifndef __EMSCRIPTEN__

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch 2 times, most recently from 2827a20 to 41df734 Compare February 24, 2026 15:23
@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch from 41df734 to cf57334 Compare March 9, 2026 18:35
@vgvassilev
Copy link
Contributor

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).
I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.
Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

Do we have an ETA for this?

@SylvainCorlay it’s been a couple of weeks can we move forward with this or the alternative is ready?

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch from e9efb75 to 93e375a Compare March 9, 2026 19:55
@mcbarton
Copy link
Collaborator Author

mcbarton commented Mar 9, 2026

Just a note in case it is brought up, the ci failing here doesn't mean we are not in a position to go. We are just waiting on a CppInterOp release to make it go green.

@JohanMabille
Copy link
Collaborator

JohanMabille commented Mar 10, 2026

On the packaging end, we will probably split the conda recipe in multiple outputs (so that installing xeus-cpp does not include the OpenMP kernelspecs unless e.g. xeus-cpp-openmp is installed).
I would also like to highlight that the Jupyter Enhancement Proposal was specifically created to address the C++ kernel use case. It has undergone extensive reviews, and our team produced a reference implementation covering kernel, server, and frontend components. The JEP is currently in the final voting phase.
Given that the official standardization process is nearly complete, it is disappointing to rush a workaround while the long-term solution is so close to being finalized and was developed specifically with the C++ kernel in mind.

Do we have an ETA for this?

@SylvainCorlay it’s been a couple of weeks can we move forward with this or the alternative is ready?

Let's move forward with this and refactor when the alternative is ready.
We can split the packages on conda-forge to avoid too many kernels for users who simply want vanilla C++.

Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

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

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 ?

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch from 93e375a to ec6207b Compare March 12, 2026 09:30
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"};
std::string resource_dir;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::string" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:25:

- #ifndef __EMSCRIPTEN__
+ #include <string>
+ #ifndef __EMSCRIPTEN__

} else {
std::cerr << "Failed to detect the resource-dir\n";
}
resource_dir = Cpp::DetectResourceDir();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "CppImpl::DetectResourceDir" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:9:

- #include <regex>
+ #include <CppInterOp/CppInterOp.h>
+ #include <regex>

@mcbarton mcbarton force-pushed the Add-OpenMP-kernel branch from ec6207b to 6debb9e Compare March 13, 2026 14:10
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.

8 participants