-
Notifications
You must be signed in to change notification settings - Fork 227
JupyterLab 3 update #272
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 3 update #272
Conversation
This pull request introduces 3 alerts and fixes 3 when merging 1a62135 into 3372f9b - view on LGTM.com new alerts:
fixed alerts:
|
1a62135
to
0d928b6
Compare
This pull request fixes 3 alerts when merging 0d928b6 into 3372f9b - view on LGTM.com fixed alerts:
|
Any idea how/why this fixes the mysterious multiple calls to init alert that showed up in the examples PR? Also, very excited for this |
I have no idea ahah.
Yes! Me too, this will be much simpler for everyone. This PR is not finished yet. I'll finish it tomorrow. |
install_requires=[ | ||
'ipykernel>=4.7', | ||
'ipywidgets>=7.5.0', | ||
'matplotlib>=2.0.0' | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on having an extra_requires: 'lab'
that pulls in jupyterlab)widgets
so that the install instructions could be:
pip install ipympl
# or for jupyterlab
pip install ipympl[lab]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although I wonder if jupyterlab_widget
will be an ipywidgets's dependency. If it's not, we could make it a dependency of ipympl, it's actually not a big dependency so we could ship it just by doing pip install ipympl
, I really want to make user's life simple from now on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asking about that on gitter awhile ago. see https://gitter.im/jupyter-widgets/Lobby?at=5f7223041c5b0d210ad89905
I think ipywidgets will not require it at least in the immediate future.
I think the issue with requiring it for ipympl is that then you pick up all of jlab as a depdency (at least till the new pip comes out). I still think that thats worth it but I recognize that there are downsides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One notable comment from that thread:
right now jupyterlab_widgets depends on jupyterlab - I suppose it doesn't have to, though, especially if the peer dependency/run constraint gives the right version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh I wouldn't have expected that it depends on jlab, I am a bit disappointed. Then I agree with your first proposition.
There might be a hacky workaround for this though, we can maybe depend on jupyterlab_widgets at buildtime, and install the required static files under share and it might work.
This pull request fixes 3 alerts when merging 24353d4 into 3372f9b - view on LGTM.com fixed alerts:
|
24353d4
to
d20967b
Compare
This pull request fixes 3 alerts when merging d20967b into 3372f9b - view on LGTM.com fixed alerts:
|
This pull request fixes 3 alerts when merging 4c4582f into 3372f9b - view on LGTM.com fixed alerts:
|
6523883
to
65f6239
Compare
This pull request fixes 3 alerts when merging 65f6239 into 3372f9b - view on LGTM.com fixed alerts:
|
65f6239
to
99cf7f4
Compare
@ianhi I think this PR is almost ready, it's working. Although let's wait for an official JupyterLab 3 release before merging and releasing. |
This pull request fixes 3 alerts when merging 99cf7f4 into 3372f9b - view on LGTM.com fixed alerts:
|
@martinRenou I couldn't get this to work. Kept running into this error:
I think that these lines need to be changed as well: ipympl/ipympl/backend_nbagg.py Lines 27 to 28 in 3372f9b
Did you delete the old files in the static directory? If you didn't that may be why it's working for you and not for me. |
Oh yeah this needs to be changed. Bad that the CI did not catch it. |
Adding tests via |
I think this in pytest.ini would do it
along wiht pytest and nbval in an extra_requires for test |
f830e64
to
5b29411
Compare
This pull request fixes 3 alerts when merging 5b29411 into 3372f9b - view on LGTM.com fixed alerts:
|
This pull request fixes 3 alerts when merging 3445cdb into 3372f9b - view on LGTM.com fixed alerts:
|
@ianhi it should work now |
So it does! 🎉 Thanks for this |
README.md
Outdated
### Install the JupyterLab extension | ||
### Use in JupyterLab | ||
|
||
In order to install the JupyterLab extension `jupyter-matplotlib`, you will first need to install `nodejs`, you can install it with `conda` doing | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things here:
-
Not sure where we landed on requiring jupyterlab-widgets (looks as though Remove JupyterLab dependency in jupyterlab_widgets jupyter-widgets/ipywidgets#2995 won't happen in the very near future?). But currently ipympl doesn't explicitly depend on it, so it ought to be included in the install instructions.
-
I agree that it is best to use jlab3 but I bet many people will still use jlab2 for awhile. For example on jlab-git there's even still been the occasional person reporting an error they encountered on jlab1. Maybe we should keep the info but in a separate file than the readme. like
installing-for-jlab2.md
and link to it from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This PR cannot be merged as it is anyway.
I suggest we wait for JupyterLab 3 official update and see what decisions are made on jupyterlab_widgets.
I agree
oooh and the dev install instructions for jlab are different now as well right? like |
Thanks for your reviews @ianhi, that's very much appreciated |
due to windows/npm issue on "fsevent", I can't compile on Windows... having an artifact generated by the CI system would be a nice feature, (like in github.com/altendky/pyqt5-tools/actions/runs/300796583) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
below suggested changes to pick up the jlab-widgets dep by way of ipywidgets which depends on it now 🎉
And enjoy vacation :)
install_requires=[ | ||
'ipykernel>=4.7', | ||
'ipywidgets>=7.5.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install_requires=[ | |
'ipykernel>=4.7', | |
'ipywidgets>=7.5.0', | |
install_requires=[ | |
'ipykernel>=4.7', | |
'ipywidgets>=7.6.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, ipywidgets 7.6 is not available in conda for python 2. If you want to support python 2, don't up this requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment @jasongrout, there is no package supporting Python 2 anymore on conda-forge, right? So there should not be any problem increasing the requirement here? The ipympl package won't be available for Python 2 anyway.
Hi, can you edit the readme to show that v0.5.8 is incompatible with jupyterlab 3? for anyone getting here from google downgrade:
then install ipympl using the instructions |
setup.py
Outdated
('share/jupyter/nbextensions/jupyter-matplotlib', | ||
'ipympl/nbextension', '*.*'), | ||
('share/jupyter/labextensions/jupyter-matplotlib', | ||
'ipympl/labextension', "*.*"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to @martinRenou the first two changes below seem to be necessary to make the resulting wheel file work.
('share/jupyter/nbextensions/jupyter-matplotlib', | |
'ipympl/nbextension', '*.*'), | |
('share/jupyter/labextensions/jupyter-matplotlib', | |
'ipympl/labextension', "*.*"), | |
('share/jupyter/nbextensions/jupyter-matplotlib', | |
'ipympl/nbextension', '**'), | |
('share/jupyter/labextensions/jupyter-matplotlib', | |
'ipympl/labextension', "**"), | |
('share/jupyter/labextensions/jupyter-matplotlib', '.', 'install.json'), |
Also I added the install.json file (https://github.com/jupyterlab/extension-cookiecutter-ts/blob/master/%7B%7Bcookiecutter.python_name%7D%7D/install.json) which will also need to be created
@shaielc @bjudkewitz I if you'd like to use ipympl wiht jlab3 before this is merged i've created a wheel that works with jlab3 based off this PR that you can install with this command:
|
small remark on ipympl-0.6.0 to be : in classic Jupyter Notebook mode, if you have forgotten to do |
Tracking this in issue #281 |
It does not show anything when you call the |
@GF-Huang that's actually expected behavior. In short it's because the very first time you call I'm happy to try to explain at greater length but lets please talk more about it on https://discourse.matplotlib.org/ so we can keep this PR about the update. |
acef3bc
to
348959b
Compare
Signed-off-by: martinRenou <[email protected]>
348959b
to
e31db70
Compare
This should be ready now (given that CI passes). Will merge and make a release around tonight. Thanks a lot @ianhi for the reviews! |
This pull request fixes 4 alerts when merging e31db70 into bd477f5 - view on LGTM.com fixed alerts:
|
Note to self: update binder config to use JupyterLab 3 |
Removing code block that is not necessary thanks to release 0.6.1 at ipympl: matplotlib/ipympl#272
Please open a separate issue, and can you add the output of |
@GF-Huang please don't comment on merged PRs in this way, as it "pings" people who commented on the thread unnecessarily. Feel free to open a new issue. Your comment about labextension is correct. |
Fix #281, fix #276, fix #268, fix #265