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 new marked version >=4.0.0 to fix #151 and support nbclassic >=0.5 #152

Closed
wants to merge 4 commits into from
Closed

Conversation

jslorrma
Copy link

@jslorrma jslorrma commented Mar 1, 2023

This fixes nbclassic >=0.5 breaks jupyter_nbextensions_configurator #151

This fixes `nbclassic >=0.5 breaks jupyter_nbextensions_configurator`
#151
@echarles
Copy link
Collaborator

echarles commented Mar 1, 2023

Thx @ lot @jslorrma for this fix. I have tested your branch and it works fine, the configurator is up-and-running again.

A comment for discussion, depending also on @@juhasch inputs: As this fix will work fine for nbclassic>=0.5 but will break for <5, but also for notebook, I wonder how this should be handled? We are already detecting in the nbclassic ui if the server is effectively nbclassic (the ui is also use by notebook 6.5) https://github.com/jupyter/nbclassic/blob/c0ced4f1472100f9de8e773af9b6cd6e254ce1be/nbclassic/static/notebook/js/about.js#L13

@echarles
Copy link
Collaborator

echarles commented Mar 1, 2023

Actually this is ok as we can ask users to upgrade to the latest nbclassic or notebook 6.5.

Copy link
Collaborator

@echarles echarles left a comment

Choose a reason for hiding this comment

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

LGTM

@echarles
Copy link
Collaborator

echarles commented Mar 1, 2023

@juhasch I don't have right to merge, could you please review? Thx a lot.

@echarles
Copy link
Collaborator

echarles commented Mar 2, 2023

@jslorrma I thought a bit more and this would break notebook 6.4. One way to move forward is to bump the dependency in setup.py to 'notebook >=6.5 and document in the README that constraintes (the current version fo notebook 6.4, the next version for notebook 6.5 and nbclassic). Is it possible for you to update your PR with this?

@jslorrma
Copy link
Author

jslorrma commented Mar 2, 2023

@jslorrma I thought a bit more and this would break notebook 6.4. One way to move forward is to bump the dependency in setup.py to 'notebook >=6.5 and document in the README that constraintes (the current version fo notebook 6.4, the next version for notebook 6.5 and nbclassic).

I was thinking in the same direction yesterday night ... 👍🏻

I it possible for you to update your PR with this?

Sure. I'm a bit busy today but as this isn't a big deal I hope I can make it until this evening. Anything to adjust and take into account for the conda package build?

README.md Outdated Show resolved Hide resolved
jslorrma and others added 2 commits March 4, 2023 02:24
Co-authored-by: Eric Charles <eric@datalayer.io>
@echarles
Copy link
Collaborator

echarles commented Mar 4, 2023

Thx for updates on README @juhasch

This sounds good to be merged now.

@echarles
Copy link
Collaborator

@juhasch what are your thoughts with this PR? Thx.

@yuvipanda
Copy link

Aha, after a bunch of digging, discovered this is what is breaking stuff for a bunch of our users :) Let us know if there is anything we can do to help move this forward.

THANK YOU FOR ALL YOUR WORK!!!

@echarles
Copy link
Collaborator

Aha, after a bunch of digging, discovered this is what is breaking stuff for a bunch of our users :) Let us know if there is anything we can do to help move this forward.

@yuvipanda Thx for raising the impact and importance of this. I think we have everything to fix that, but are missing eyes from maintainers cc @damianavila @juhasch

@echarles
Copy link
Collaborator

This has been discussed at the notebook developer meetings and followed in mail thread, see

Raising that again at today Notebook call right now.

yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request Mar 22, 2023
jupyter-nbcontrib-extensions required the following fixes to
work:

1. Pin nbclassic<0.5 (done in
   2i2c-org/utoronto-image#49, until
   Jupyter-contrib/jupyter_nbextensions_configurator#152
   is merged and released)
2. Upgrade jupyter-nbcontrib-extensions to latest released version
   (done in 2i2c-org/utoronto-image#48)
3. Stop using ServerApp, and use NotebookApp (issue to be filed here)

Ref 2i2c-org/utoronto-image#48
@echarles
Copy link
Collaborator

echarles commented Mar 23, 2023

We have just got merge rights on this repo and still need permissions to pypi for the release.

In the meantime, we should open a PR without any change on marked side that defines upper boundaries for notebook and nbclassic, merge that PR, release, and then only merge these changes.

WDYT?

@yuvipanda
Copy link

@echarles that seems reasonable!

Note that there are further jupyter_server related issues #153. I tested by installing from this branch but that does not seem to fix it.

@juhasch
Copy link
Contributor

juhasch commented Mar 26, 2023

@echarles Sorry for not responding. LGTM

Do you need anything from me, should I upload it to PyPi after merge ?

@echarles
Copy link
Collaborator

@juhasch I have finally opened jupyter/nbclassic#236. Once merged in nbclassic and release, the changes proposed here should not be necessary. I keep you updated.

@echarles
Copy link
Collaborator

jupyter/nbclassic#236 is merged. I will cut a new nbclassic release, so this PR is not needed anymore.

@echarles echarles closed this Mar 29, 2023
@echarles
Copy link
Collaborator

nbclassic 0.5.4 is released so closing this PR.

Do you need anything from me, should I upload it to PyPi after merge ?

@juhasch it would be good to confirm that we have enough people having acces to pypi for the releases. e.g. has @damianavila the access. Feel free to add me also.

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.

4 participants