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

Shim to support the notebook extensions #113

Merged
merged 5 commits into from Jun 27, 2022

Conversation

echarles
Copy link
Member

@echarles echarles commented Jun 14, 2022

This PR add a shim to ensure the classic notebook extension installed on top of nbclassic do not break while importing notebook packages being no more available in nbclassic. To achieve this backwards compatibility, the needed module/package are shimmed, based on the existence or not of specific versions of notebook (V6, >V7).

@echarles
Copy link
Member Author

For classic extensions to be operational, we may also need jupyter-server/jupyter_server#868 (add back IPythonHandler) and depend on a new jupyter-server release with that patch.

@echarles echarles added the enhancement New feature or request label Jun 14, 2022
@echarles echarles marked this pull request as ready for review June 15, 2022 04:38
@echarles
Copy link
Member Author

We have discussed this PR and the options to ensure backwards compatibility of the notebook extensions for nbclassic at last community call with @Zsailer and @afshin . I have further discussed the options with @ericsnekbytes , @RRosio and @seibert.

My last commit takes into account the feedback to not add IPythonHandler into jupyter-server. There are advantages to progress with the option of this PR to:

  1. Have a single place where the shim occurs. To further adopt the idea to not inject shim in non-related repos like jupyter-server, it sounds good and in-line to also not inject shims in the notebook repos.
  2. We should avoid allowing extensions developers of notebook 7 to use the old notebook packages (notebook.base..) as we want at some point to remove that shim. In other words, if we allow them to use eg. notebook.base..., those extensions will break when the shim that would be in notebook 7 will be removed (e.g. in notebook 8)

I will demo this PR at today community call with various setups (nbclassic with a working notebook extension while notebook 7, or notebook 7 or no notebook is installed).

cc/ @kevingoldsmith

@echarles
Copy link
Member Author

echarles commented Jun 22, 2022

@Zsailer We have just discussed the options at the community call and would love you feedback on the option proposed in this PR. I have given the demo and will be happy to replay it in one-one if you like.

We have decided to re-discuss this during next week community call. Of course, we don't need to wait that call to further brainstorm and exchange on this 👍

@Zsailer
Copy link
Member

Zsailer commented Jun 24, 2022

  1. Have a single place where the shim occurs. To further adopt the idea to not inject shim in non-related repos like jupyter-server, it sounds good and in-line to also not inject shims in the notebook repos.
  2. We should avoid allowing extensions developers of notebook 7 to use the old notebook packages (notebook.base..) as we want at some point to remove that shim. In other words, if we allow them to use eg. notebook.base..., those extensions will break when the shim that would be in notebook 7 will be removed (e.g. in notebook 8)

👍 Totally agree with these two goals.

I think this approach is good. Let's roll with it. If we hear reports of major issues coming from patching sys.modules, we can always change course. In the meantime, this is a nice, clean solution. 🚀

Great work, @echarles!

@echarles
Copy link
Member Author

Super! Thx a log @Zsailer for the review and though with this. I see relevant CI failure due to the usage of assert. Let me fix that before merging 👍

@echarles
Copy link
Member Author

The failing selenium tests are also encountered in #114. This is prolly a regression and not related to this PR.

I have fixed the relevant tests, so this is good to go. Merging.

@echarles echarles merged commit 83a2fe8 into jupyter:main Jun 27, 2022
@echarles
Copy link
Member Author

I have opened #114 to follow-up on the selenium tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants