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 #15

Merged
merged 1 commit into from
May 12, 2023

Conversation

peytondmurray
Copy link
Collaborator

This PR adds Jupyterlab 4 support for jupyterlab-skip-traceback. I tested this out with 4.0.0b1 and everything seemed to work:

image

There are some minor cosmetic changes I noticed from 3.x due to changes in global styles; the most noticeable one that I needed to fix was the size of the copy button, which appeared much larger in 4.x than in 3.x. Aside from that there's a slight difference in transparency of the caret and the copy buttons - I'm hoping that going with the new 4.x styles is acceptable, but if not I can make the change to match the old styles.

@peytondmurray peytondmurray marked this pull request as ready for review April 20, 2023 00:07
Copy link

@ankit-gautam23 ankit-gautam23 left a comment

Choose a reason for hiding this comment

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

LGTM

@singharpit94 singharpit94 self-requested a review April 28, 2023 04:23
Copy link

@singharpit94 singharpit94 left a comment

Choose a reason for hiding this comment

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

I have added few comments. Couple of important things which is blocking me to test out the changes locally

I am not able to run pip install -e . without a setup.py file, it complains with the below error:

ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /codemill/singhar/jupyterlab-skip-traceback
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

Also when I run pip install . (without editable mode install), I run into the below error
ValueError: Extensions require a devDependency on @jupyterlab/builder@^3.6.2, you have a dependency on 4.0.0-rc.0

I checked my node_modules in my venv and it does has @jupyterlab/builder@ 4.0.0-rc.0, do I need to upgrade some other packages to get this working, any leads here?

python -m build
pip uninstall -y "jupyterlab-skip-traceback" jupyterlab

- name: Upload extension packages

Choose a reason for hiding this comment

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

Do we need the upload as part of the github workflow or we keep it only when release this package.

Copy link
Collaborator Author

@peytondmurray peytondmurray Apr 28, 2023

Choose a reason for hiding this comment

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

The artefacts are just used in the test_isolated step. This could in principle be condensed into a single job, but it's the default for the ts-extension-cookiecutter repo, so I kept it. It's really up to you if you want to get rid of test_isolated 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

☝️ To be clear this has nothing to do with releasing the package, this is only used during the CI job. We're just uploading the extension to temporary github storage for the duration of the job.

"**/*.d.ts",
"tests"
],
"prettier": {

Choose a reason for hiding this comment

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

Any strong reason to move eslint and prettier to package.json instead of keeping them in seperate files. from the extension upgrade in https://github.com/jupyterlab/extension-examples/pull/225/files I do think think this is mandatory to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's definitely not mandatory, and there's no strong reason to do so other than reducing the amount of stuff cluttering up the repository. I'm not a big fan of needing loads and loads of boilerplate files clogging the repo as is the usual for the extension cookiecutter, but if you prefer these to be broken out into separate files then I'm happy to do so.

src/index.ts Outdated Show resolved Hide resolved
@peytondmurray
Copy link
Collaborator Author

As far as your build issues go:

  1. What version of everything are you using? I think editable builds have been in this build backend for a while. Can you try using a more recent toolchain?
  2. 99% sure this is something to do with you having old build stuff interfering with the current build. Do a git clean -fdx to clear everything out. Then start from a new virtualenv and try building again.

@singharpit94
Copy link

As far as your build issues go:

  1. What version of everything are you using? I think editable builds have been in this build backend for a while. Can you try using a more recent toolchain?
  2. 99% sure this is something to do with you having old build stuff interfering with the current build. Do a git clean -fdx to clear everything out. Then start from a new virtualenv and try building again.

Yes, I was able to use the upgraded extension after fixing build issues at my end.

@peytondmurray
Copy link
Collaborator Author

@singharpit94 @ankit-gautam23 I think this is ready to be merged. Is there anything else left to do here?

@ankit-gautam23
Copy link

@peytondmurray build issue was the biggest concern as @singharpit94 has solved it so I think we are good to go.

@peytondmurray
Copy link
Collaborator Author

@ankit-gautam23 or @singharpit94 I don't have merge rights, can you approve the workflow and merge when it passes?

@singharpit94
Copy link

Good from my end as well. One minor thing, can you update the README too with the latest changes, post that we can merge

1 similar comment
@singharpit94
Copy link

Good from my end as well. One minor thing, can you update the README too with the latest changes, post that we can merge

@singharpit94
Copy link

@peytondmurray I think we will need some changes wrt to bumping the version in package.json, changelog. Also few tests are failing, can you take a look?

@peytondmurray
Copy link
Collaborator Author

Oh interesting - the link checker already caught some failing links to invalid tags. Nice!

@peytondmurray peytondmurray force-pushed the jupyterlab-4-upgrade branch 2 times, most recently from 812e2eb to c60cbd6 Compare May 11, 2023 05:57
@peytondmurray
Copy link
Collaborator Author

@singharpit94 Okay, README.md, package.json, and CHANGELOG.md were modified to bump the version and fix the link test.

@singharpit94
Copy link

@peytondmurray check links still failing, do we need to release first or update somewhere else to get this build green?

@peytondmurray
Copy link
Collaborator Author

peytondmurray commented May 11, 2023

@peytondmurray check links still failing, do we need to release first or update somewhere else to get this build green?

For the CHANGELOG.md failure in principle yes, because the 5.0.0 release has not been tagged yet. Or we could just update CHANGELOG.md after we merge this PR and tag a release.

For the README.md failure, it looks like something is wrong with the binder link. Not sure what to do about this, I'm getting just a generic 500 internal server error so I assume this is an issue with mybinder.org right now.

The build error is weird, I'm still debugging it; let me get back to you...

@peytondmurray
Copy link
Collaborator Author

Huh, so the extension template used in the upgrade incorrectly generates the wheel names for the github build action when uploading/downloading artifacts. I'll report this as a bug on the jupyterlab repo.

@peytondmurray
Copy link
Collaborator Author

@singharpit94 @ankit-gautam23 Looks like tests are now passing; after we merge this I'll make another PR to update CHANGELOG.md.

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