Skip to content

Add support for Windows in the Jupyter kernel for Cling #19369

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

yjoer
Copy link

@yjoer yjoer commented Jul 14, 2025

This PR adds support for Windows in the Jupyter kernel. On Windows (MSVCRT), the stdout and stderr file descriptors are not exported as global symbols. To work around this, we now retrieve them using _fdopen. Beginning with Python 3.12, we can utilize the new cross-platform API os.set_blocking to configure pipes as non-blocking.

Since Windows does not offer a direct equivalent to the "select" system call for pipes, I replaced it with PeekNamedPipe to check for available data in the pipe. Unlike select, PeekNamedPipe does not allow efficient blocking. As a result, we currently poll every 0.25 seconds, aligning with the flush interval to check for new data and act accordingly.

The outcome looks appealing, with the interpreter's output and errors being correctly streamed.
image

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@yjoer yjoer requested review from bellenot and dpiparo as code owners July 14, 2025 22:17
@guitargeek
Copy link
Contributor

This is amazing! Can we maybe get test coverage for this by enabling some tests from here?

https://github.com/root-project/root/blob/master/roottest/python/JupyROOT/CMakeLists.txt#L1

With your development, at least the ROOT_kernel.ipynb should work, no?

Copy link

Test Results

    21 files      21 suites   3d 8h 23m 56s ⏱️
 3 203 tests  3 203 ✅ 0 💤 0 ❌
65 591 runs  65 591 ✅ 0 💤 0 ❌

Results for commit 54ad394.

@yjoer
Copy link
Author

yjoer commented Jul 15, 2025

I'm getting a strange error when using std::cout with the dynamic library, but it doesn't happen when I run the same code in the Cling CLI. It's got me scratching my head. Do you know what's missing?

image image

@bellenot
Copy link
Member

@yjoer FYI, some symbols have to be explicitly exported. See for example core/metacling/src/CMakeLists.txt#L129-L215 and interpreter/cling/tools/driver/CMakeLists.txt#L44-L105

@yjoer
Copy link
Author

yjoer commented Jul 15, 2025

This is incredible, thank you! I aligned the symbols with the ones in tools/driver/CMakeLists.txt#L44-L105
and got it working.

image

@yjoer
Copy link
Author

yjoer commented Jul 15, 2025

With your development, at least the ROOT_kernel.ipynb should work, no?

I believe that's meant for a different kernel. This one doesn't support %cpp or %python magic commands.

@yjoer
Copy link
Author

yjoer commented Jul 15, 2025

I managed to run most parts of ROOT_kernel.ipynb that do not interact with the ROOT object.

jpt-cling.mp4

@guitargeek
Copy link
Contributor

guitargeek commented Jul 16, 2025

Super cool! Why does the rest of the notebook not work? You think that can still be fixed?

What would be important for this PR is to have test coverage of what you developed, and the first half of that ROOT_kernel.ipynb notebook seems to do the job. How do you think we should proceed? Maybe split up the ROOT_kernel.ipynb in two parts, one part that works also on Windows now and then the second half that uses the ROOT module? Then we can enable the first part to run on Windows in the CI. Of course, supporting one whole notebook would be even better, but I don't want to ask too much of you.

@yjoer
Copy link
Author

yjoer commented Jul 16, 2025

The kernel updated here is located at https://github.com/root-project/root/tree/master/interpreter/cling/tools/Jupyter, which differs from the one that supports the root framework. I am not aware of any existing test setup for this specific kernel, either.

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.

3 participants