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

Jupyterlab 4 upgrade #83

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Conversation

peytondmurray
Copy link
Collaborator

@peytondmurray peytondmurray commented Mar 10, 2023

This PR adds support for Jupyterlab 4. Since we need to upgrade jupyterlab and lumino dependencies for this, I've also taken the chance to upgrade other dependencies as well. With the exception of typescript (which is set to 4.7, the version used by jupyterlab), all packages that can be bumped with a yarn --latest upgrade-interactive have been bumped. The Jupyterlab dependencies has been manually upgraded to 4.0.0-alpha21. I also updated the project metadata to include python 3.10 and 3.11 in the tags.

Here's the extension running in a fresh environment with jupyterlab==4.0.0a35:

image

In the process of writing this PR, jupyterlab switched to using yarn "berry" as the js package manger, i.e. yarn==3.5.0. This meant that there were some additional files to be ignored that I added to .gitignore; these have to do with the new version of yarn's dependency caching system. Finally, I also needed to add a .yarnrc.yml to configure yarn to use node-modules, because jlpm fails to build the extension otherwise.

I also updated the github actions used by the project to their latest versions.

Finally, I added an additional kwarg to the the call to npm_builder in setup.py to ensure jlpm is used; by default, it uses whatever yarn is available on the user's PATH, which can cause build issues in certain cases, notably when yarn@1 is installed globally and jlpm@3 (that is, yarn@3) is installed in the python environment.

Todo

  • Manual testing

@mlucool
Copy link
Member

mlucool commented Mar 10, 2023

With the exception of typescript (which is set to 4.7, the version used by jupyterlab),

Jupyterlab@5 is moving to TypeScript@ 5 - jupyterlab/jupyterlab#14114. Lets bump that up too.

@peytondmurray
Copy link
Collaborator Author

Sounds good, I've bumped it to 5.0.1-rc.

@peytondmurray
Copy link
Collaborator Author

Okay, local testing is good, so this seems ready for review.

@peytondmurray peytondmurray marked this pull request as ready for review March 14, 2023 22:07
@peytondmurray peytondmurray changed the title [WIP] Jupyterlab 4 upgrade Jupyterlab 4 upgrade Mar 14, 2023
@peytondmurray
Copy link
Collaborator Author

peytondmurray commented Mar 20, 2023

@krassowski Could you retrigger the CI? I think the build was failing before because jupyterlab==3.x was being installed on the runner instead of the latest 4.0.0a37.

@peytondmurray peytondmurray force-pushed the jupyterlab-4-upgrade branch 2 times, most recently from 30578a5 to 1cd2dd4 Compare March 20, 2023 20:56
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

This is very close to the finish line, just two minor suggestions.

setup.py Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mlucool
Copy link
Member

mlucool commented Mar 23, 2023

I'm surprised the build worked in the GH action as on two different OS, I get the following:

/path/to/jupyterlab-execute-time (jupyterlab-4-upgrade) $ jlpm run clean:lib
Internal Error: jupyterlab-execute-time@workspace:.: This package doesn't seem to be present in your lockfile; run "yarn install" to update the lockfile
    at P0.getCandidates (/path/to/jupyterlab-execute-time/.venv/lib/python3.10/site-packages/jupyterlab/staging/yarn.js:435:5145)
    at yf.getCandidates (/path/to/jupyterlab-execute-time/.venv/lib/python3.10/site-packages/jupyterlab/staging/yarn.js:391:1264)
    at /path/to/jupyterlab-execute-time/.venv/lib/python3.10/site-packages/jupyterlab/staging/yarn.js:439:7693
    at of (/path/to/jupyterlab-execute-time/.venv/lib/python3.10/site-packages/jupyterlab/staging/yarn.js:390:11070)
    at ge (/path/to/jupyterlab-execute-time/.venv/lib/python3.10/site-packages/jupyterlab/staging/yarn.js:439:7673)

Was the lockfile updated when you did other changes? Any idea on where "workspace" imports are coming from?

I had yarn@1 on these, so I wonder if there is some bug in jlpm that's not using the venv one at times? That's a shot in the dark guess with little evidence.

@peytondmurray
Copy link
Collaborator Author

Arrgh, this must be something related to the yarn update. Let me try building from scratch to see if I can't reproduce the issue.

@peytondmurray
Copy link
Collaborator Author

I just tried to reproduce this. Here's what I did, starting from a fresh python 3.11.2 virtualenv:

git clean -fdx  # Remove all build artefacts
pip install .
pip install jupyterlab==4.0.0a37

And it seemed to build without any issues. My system is using

Node: v18.15.0
Yarn: v3.5.0

in case that matters. If you can't get it to build, I can also try to construct a conda environment to see if it's some other dependency issue.

@mlucool
Copy link
Member

mlucool commented Mar 23, 2023

This was solved worked around via (this has to do with having yarn@1 in the path):

diff --git a/setup.py b/setup.py
index 14f4b1a..a134904 100644
--- a/setup.py
+++ b/setup.py
@@ -66,7 +66,7 @@ setup_args = dict(
 
 
 post_develop = npm_builder(
-    build_cmd="install:extension", source_dir="src", build_dir=lab_path
+    build_cmd="install:extension", source_dir="src", build_dir=lab_path, npm='jlpm'
 )
 setup_args["cmdclass"] = wrap_installers(
     post_develop=post_develop, ensured_targets=ensured_targets
diff --git a/yarn.lock b/yarn.lock

But that's an issue for another time.

This has a bug that previous execute times don't seem to load on notebook load:
execute_time

@peytondmurray
Copy link
Collaborator Author

@mlucool Yes, and I've also noticed that execute times fail to load for cells with import statements. Do you want to tackle these here or in a separate issue?

@mlucool
Copy link
Member

mlucool commented Mar 23, 2023

execute times fail to load for cells with import statements

This had no import statement.

This seems like a regression due to this upgrade, so let's ensure this PR is updated until it works at least as good as it did for lab@3.

@peytondmurray
Copy link
Collaborator Author

This had no import statement.

Yeah, this was just something else I noticed when using the extension.

This seems like a regression due to this upgrade, so let's ensure this PR is updated until it works at least as good as it did for lab@3.

Sounds good, I'll look into it.

@peytondmurray
Copy link
Collaborator Author

Incredibly weird: I can recreate this bug, but when I put a console logging statement inside the createNew function, the issue goes away. Maybe this is some weird race condition where the log statement delays drawing the page or something? I'll continue investigating...

@peytondmurray
Copy link
Collaborator Author

For some reason, CodeCell instances which are not ready don't have inputArea attributes. What this meant was on first load, the extension was trying to attach cell metadata to the cells, which didn't yet have inputAreas. I'm guessing this is some kind of lazy loading thing that speeds up first page paint, but anyway I added an await cell.ready; before attaching the new DOM nodes that display the execution metadata, and that seemed to do the trick. @krassowski Can you take a second look? I want to make sure I'm not misunderstanding something here.

@peytondmurray
Copy link
Collaborator Author

After a discussion with @krassowski it seems like this approach is right. This should be good to merge.

@mlucool mlucool merged commit 9f0b205 into deshaw:master Mar 29, 2023
@mlucool
Copy link
Member

mlucool commented Mar 29, 2023

Thanks!

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.

None yet

3 participants