Skip to content
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

Add support for building against JupyterLab 4 and Lumino 2 #3752

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Apr 4, 2023

Follow up #3707

Fix #3790

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Binder 👈 Launch a binder notebook on branch martinRenou/ipywidgets/lab4_lum2

@echarles
Copy link
Contributor

echarles commented Apr 5, 2023

@martinRenou I was just going to take over #3707 which was working apart the galata test (now has conflicts).

Question around dependencies: You are giving choice to javascript in package.json (e.g. "@jupyterlab/services": "^6.0.0 || ^7.0.0-beta.0" - "@lumino/coreutils": "^1.11.1 || ^2"), however the python version is pinned to verison 4 of jupyterlab (jupyterlab ==4.0.0b0). We already discussed that a bit, could you bring more details around that as my first feeling is that there is a risk on inconsistency, but I may miss background here.

@martinRenou
Copy link
Member Author

Thanks for commenting. I wanted to debug a bit on this.

however the python version is pinned to verison 4 of jupyterlab (jupyterlab ==4.0.0b0)

In tests yes. To make sure we test against the latest jlab in the ui-tests.

my first feeling is that there is a risk on inconsistency

There shouldn't be any issue coming from the the choice we give in the JavaScript packages versions. This is just giving more freedom on the downstream packages that depend on @jupyter-widgets packages (like Voila, which is currently stuck with @lumino 1 and @jupyterlab 3 due to ipywidgets). But the ipywidgets package should be built with Lumino 2 and Lab 4 and the built output published.

However, yarn is being annoying (again) here and pulls both Lumino 1 and Lumino 2 packages in the dev installation for no obvious reason. I'm a bit stuck now because of this.

@martinRenou
Copy link
Member Author

martinRenou commented Apr 6, 2023

However, yarn is being annoying (again) here and pulls both Lumino 1 and Lumino 2 packages in the dev installation for no obvious reason. I'm a bit stuck now because of this.

This seems to be because the:

  • "@lumino/widgets": "^1 || ^2" constraint resolves to "^2" (perfect)
  • "@jupyterlab/application": "^3.6 || ^4.0.0-alpha.0" constraint resolves to ^3.6, not "^4.0.0-alpha.0"! (WHAT)

This means that "@jupyterlab/application" will pull @lumino/widgets version 1 -> We end up with two Lumino versions in the node_modules -> The typescript compilation cannot succeed.

Weirdly enough, the "@jupyterlab/application": "~3 <3.6 || ^4.0.0-alpha.0" constraint would resolve to 4.0.0-alpha.0, and the "@jupyterlab/application": "~3 <3.7 || ^4.0.0-alpha.0" constraint would resolve to 3.6, so the 3.6 version may be the culprit??? I have no clue what yarn is doing here.

@martinRenou martinRenou force-pushed the lab4_lum2 branch 4 times, most recently from 3433707 to 057de2a Compare May 17, 2023 14:20
@martinRenou
Copy link
Member Author

We'll be stuck in the docs by Jupyterlab-myst

@martinRenou
Copy link
Member Author

TODO:

Check that #3785 is fixed with this PR

@ndmlny-qs
Copy link

@martinRenou I just checked and these changes do not fix #3785 or #3781. You mentioned above

However, yarn is being annoying (again) here and pulls both Lumino 1 and Lumino 2 packages in the dev installation for no obvious reason. I'm a bit stuck now because of this.

I think I've figured out what is happening here. There are several package.json files that request lumino pacakages with versions that do not exist. For instance, python/widgetsnbextension/package.json is requesting @lumino/messaging: "^1.10.1 || 2.1. However, the latest published version for @lumino/messaging is 2.0.0. I think yarn is searching for 2.1, doesn't find it, and reverts to the 1.x version. Changing the places where a lumino package is not yet at version 2.1, but is at 2.0, prevents yarn from pulling in 1.x versions. I tried this locally and it works.

Unfortunately it still does not solve #3785 or #3781(using my local build) and jlab4, which I suspect are CSS issues needing to be resolved.

@martinRenou martinRenou changed the title Lab 4 Lumino 2 Add support for building against JupyterLab 4 and Lumino 2 Jun 19, 2023
@martinRenou
Copy link
Member Author

We really need a bot to update snapshots 😓

Copy link
Contributor

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @martinRenou. I spotted two places where Lab4 specific code is used.

python/jupyterlab_widgets/src/manager.ts Outdated Show resolved Hide resolved
python/jupyterlab_widgets/src/manager.ts Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member Author

Thanks a lot for the review @fcollonval !

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like all of these image changes added 1 px vertically. Was that due to something inherent in lumino2/JL4 ? If it is, then this change is just a downstream effect from the lumino2/JL4 change. If not, then this PR shouldn't cause updated images.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect this comes with the galata and JupyterLab 4 update.

Since this PR is not touching CSS code nor visual JavaScript code I guess it's fine to just update the references to match what the UI-tests are getting.

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking about this further. Do the unit tests also test against JL 3.6? will there be an oscillation of failures depending on which is tested against?

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated galata which comes from JupyterLab 4, so it's only testing JupyterLab 4 from now on

@trungleduc
Copy link
Contributor

Hi all, is there anything else that needs to be fixed or this PR is ready?

@martinRenou
Copy link
Member Author

All good on my side! Thank you for the review

@SylvainCorlay
Copy link
Member

Hey @afshin do you have a bit of time to review the lumino bits of this PR?

@paddymul
Copy link
Contributor

paddymul commented Jun 23, 2023 via email

@SylvainCorlay SylvainCorlay merged commit 311d83a into jupyter-widgets:main Jun 26, 2023
@martinRenou martinRenou deleted the lab4_lum2 branch June 26, 2023 13:42
@martinRenou
Copy link
Member Author

What is the ongoing ipywidgets story for 3.6? Since we removed integration testing against 3.6, how can library authors have confidence their code will work against 3.6 and 4.0?

We could indeed add another CI workflow for testing against JupyterLab 3.x.

@trungleduc
Copy link
Contributor

🚀 🎉 💯

@martinRenou
Copy link
Member Author

meeseeksdev please backport to 7.x

@lumberbot-app
Copy link

lumberbot-app bot commented Sep 4, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 7.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 311d83a352783bb8b7c1348e49be3bdb1e3724aa
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3752: Add support for building against JupyterLab 4 and Lumino 2'
  1. Push to a named branch:
git push YOURFORK 7.x:auto-backport-of-pr-3752-on-7.x
  1. Create a PR against branch 7.x, I would have named this PR:

"Backport PR #3752 on branch 7.x (Add support for building against JupyterLab 4 and Lumino 2)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants