-
Notifications
You must be signed in to change notification settings - Fork 16
Rebuild with latest jupyterlite-xeus #71
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?
Conversation
Has the issue noticed in #69 (comment) been fixed in the meantime? |
Yes, if I'm correct, jupyterlite/xeus#266 should be fixing this issue |
But this change was already part of While #69 (comment) was noticed on August 13th, so I guess we were already using that version? From https://github.com/jupyter/try-jupyter/actions/runs/16919758940/job/47942661247?pr=69 ![]() |
Ah I thought this was related to the possible incompatibility between xeus-cpp and some Python packages. Since this looks like a xeus-cpp specific issue, we should pin xeus-cpp to the working version. It looks like the jupyterlite-xeus version has nothing to do with this issue? |
You can try the RTD preview for this PR, the issue is still there: https://try-jupyter--71.org.readthedocs.build/en/71/lite/lab/index.html?path=notebooks%2Fcpp-third-party-libs.ipynb |
Friendly ping @anutosh491. Do you know which xeus-cpp version does not have this bug? |
Technically I am still confused if this is a bug with xeus-cpp. nothing really changed as such there :( But yeah a working combination would be this !
|
This did not work unfortunately. The deployed build (that works) was using jupyterlite-xeus 4.0.5. Trying this version now to see if the bumping of jupyterlite-xeus is indeed the culprit. If it's not, I don't think it should be a blocker to update jupyterlite-xeus and merge this PR, we'd need to debug this issue separately from this PR. |
It seems to be the blocker indeed, with jupyterlite-xeus 4.0.5 it does work. Having a look! |
Thanks a lot for looking into this (I gave it a try playing around with multiple xeus/xeus-cpp/cppinterop/symengine versions some days back and I couldn't spot an obvious error) |
I have a feeling I was actually completely wrong here. This PR may actually be the culprit. Basically, since this PR we don't preload the dynamic libraries for other kernels that Python. Which apparently is not a good idea for xeus-cpp since it doesn't load the dynamic libraries automatically upon If this is confirmed, I will revert this PR and make a patch release of jupyterlite-xeus. We need a better story around loading dynamic libraries... |
Confirmed. Patch release incoming 🚢 |
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.
Thanks for this, should be ready I suppose !
This also broke the |
Yeah that's pretty bad, it should work now though @IsabelParedes |
Nice, thanks all for working on this and fixing the issue! I'll do a quick smoke test before merging, but otherwise we probably need to fix #70 some time soon to catch such issues more easily. |
I've been shy and didn't revert the full changes from the PR jupyterlite/xeus#266 I'll debug more, but will probably revert it all the way. Those UI-tests should be at the jupyterlite-xeus level probably. |
Yes probably better there indeed 👍 I was thinking otherwise that the UI tests could just try to open all the notebooks, execute all of the cells for each notebook, wait for them to complete, and check that there is no error anywhere in the notebooks. Then it could assume that everything is (hopefully) working fine if that's the case. |
No description provided.