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

Compatibility with ipywidgets v8 #3930

Merged
merged 8 commits into from
Jan 10, 2023
Merged

Conversation

abdelq
Copy link
Contributor

@abdelq abdelq commented Oct 20, 2022

Attempt at fixing #3686 while being compatible with the previous version.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or modified existing tests.
  • For a new feature, I have added documentation examples in an existing or new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

@vikramaditya91
Copy link

Could you please suggest how one can test this.
The standard install python -m pip install 'git+https://github.com/abdelq/plotly.py.git@ipywidgets-8#egg=plotly' did not work as it complained with this error: #3669

I thus did a python -m pip install 'git+https://github.com/abdelq/plotly.py.git@ipywidgets-8#egg=plotly&subdirectory=packages/python/plotly'
With this, I did not get the type object 'DOMWidget' has no attribute '_ipython_display_' error anymore and I was able to plot a very simple FigureWidget with:

import plotly.graph_objects as go

f = go.FigureWidget()
f

However, I do not think it could plot a FigureWidget with some scatter in it

I think I would also somehow need to install the changes that you made in javascript right?
Could you please let me know how I can do that? Thanks

@abdelq
Copy link
Contributor Author

abdelq commented Oct 20, 2022

This is what I did when building/installing the JupyterLab extension:

cd packages/javascript/jupyterlab-plotly/
npm install
npm run build:dev
jupyter labextension install .

I would recommend doing your tests in a virtual environment.

@nicolaskruchten
Copy link
Contributor

Thanks very much for this PR! I'll test it and try to do a release next week.

Question: is this PR intended to be compatible with v8 and also backwards-compatible with v7.6?

@abdelq
Copy link
Contributor Author

abdelq commented Oct 20, 2022

Thanks Nicolas.

Question: is this PR intended to be compatible with v8 and also backwards-compatible with v7.6?

Correct. Should work for both.

@vikramaditya91
Copy link

vikramaditya91 commented Oct 21, 2022

@abdelq Besides the
python -m pip install 'git+https://github.com/abdelq/plotly.py.git@ipywidgets-8#egg=plotly&subdirectory=packages/python/plotly'

I also did the

git clone https://github.com/abdelq/plotly.py.git
git checkout ipywidgets-8
cd plotly.py/packages/javascript/jupyterlab-plotly/
npm install
npm run build:dev
jupyter labextension install .

After that I followed this:
https://plotly.com/python/figurewidget-app/

If I just wanted to display the
g = go.FigureWidget(data=[trace1, trace2], layout=go.Layout( title=dict( text='NYC FlightDatabase' ), barmode='overlay' )), then it worked well.
However, using it in combination with the widgets.HBox, did not show the FigureWidget at all.
i.e it displayed the container and not container2.

I am no expert in JS/TS but there were no errors on the console either

jupyter_client         7.4.3
jupyter_core           4.11.2
jupyter-server         1.21.0
jupyterlab             3.4.8
jupyterlab-pygments    0.2.2
jupyterlab_server      2.16.1

Maybe I am just setting them up wrongly...
Could you please confirm if this works on your end: https://plotly.com/python/figurewidget-app/

@abdelq
Copy link
Contributor Author

abdelq commented Oct 21, 2022

I set up everything from scratch and ran the example notebook.
Works fine on both versions of ipywidgets.

@vikramaditya91
Copy link

I suppose the issue is about plotly.FigureWidget not working exactly as expected, and with your fix it does work even with ipywidgets set to 8. So, I would be happy to see this merged

@vikramaditya91
Copy link

Hi. I would appreciate if this could be merged.
Is there something we are waiting for before merging this?

@nicolaskruchten
Copy link
Contributor

Just need to do review and QA :)

@nicolaskruchten
Copy link
Contributor

Question re JupyterLab 2.x ... At the moment 2.x is supported with ipywidgets 7.x. Does this PR in principle change that? Have you been able to test it? It's a bit of a pain as you have to build the Javascript extension and then install it separately. I will try to do this on my end but I wanted to flag this as a concern in case you know ahead of time that it's not supported with this PR.

@abdelq
Copy link
Contributor Author

abdelq commented Oct 26, 2022

Question re JupyterLab 2.x ... At the moment 2.x is supported with ipywidgets 7.x. Does this PR in principle change that? Have you been able to test it?

It looks like it works? However console errors are present, namely:
No provider for: jupyter.extensions.jupyterWidgetRegistry.
and a bunch of:
Exception opening new comm
when running cells.

I think this is worth looking at on your end too, as the former error might be an actual issue.

@nicolaskruchten
Copy link
Contributor

Thanks! I need to do a release today but I will really try to get this PR merged and released next week.

@aliallday
Copy link

aliallday commented Oct 27, 2022

@abdelq it looks like your modifications correctly allow a FigureWidget display but it doesn't seem like the widget redraws when the data changes. Here's a simple example to try

In one cell in jupyter lab run this:

import plotly.graph_objects as go

f = go.FigureWidget()
f.add_scatter(y=[2, 1, 4, 3]);
display(f)

and then in another cell run this:
f.data[0]['y']=[1, 1, 1, 1]

The command in the second cell doesn't update the output of the first cell. I believe in earlier versions of ipywidgets it did. (update : I just checked it on ipywidgets 7.7.1 and the second cell updated the output of the first cell)

@abdelq
Copy link
Contributor Author

abdelq commented Oct 30, 2022

After some debugging, processMessage in RenderedPlotly that is now used instead because of mime rendering is not calling processLuminoMessage in the view.

Actually, I don't even think FigureView is used to begin with...


This is getting tricky. After more digging, the way mime rendering is done is quite different from the application plugin.

Also, there is an issue that is related to mime rendering and updates (see #3053)

Right now, doing something like this will update the original plot:

import plotly.graph_objects as go

f = go.FigureWidget()
f.add_scatter(y=[2, 1, 4, 3])
x = display(f, display_id=True)
f.data[0]['y'] = [1, 1, 1, 1]
x.update(f)

For the short term, I think getting it to use the application plugin like before is probably the way to go.

@Alexboiboi
Copy link

Sorry to ask, but is there any progress going to be made soon? ;)

@pmarcellino
Copy link

Just checking in here. I would be very excited to be able to use ipywidgets v8. Is this branch functional/going to be merged in?

I think this would also solve #3686

@abdelq
Copy link
Contributor Author

abdelq commented Dec 8, 2022

I've been using this at work as it is and it's working fine.
Ping @nicolaskruchten

@nvaytet
Copy link
Contributor

nvaytet commented Dec 15, 2022

Hi, checking in here also, would be great to see this fix released. Thanks!

@nicolaskruchten
Copy link
Contributor

OK, I'm going to try to get this PR into the 5.12 release next week! Thanks for the contributions and the patience here :)

If anyone wants to help me QA this and kick the tires, you can install the pre-release build artifacts from CircleCI and report back here if there are any problems. I'm specifically concerned about backwards-compatibility with older versions of ipywidgets and Jupyter Lab/Notebook and other notebook-like environments that support widgets. Our documentation today recommends ipywidgets 7.6 or higher so I'm not that concerned about older versions than that at this time.

Thanks in advance!

@maartenbreddels
Copy link
Contributor

I tried this in notebook 6.4.8 and lab 3.2.9 with ipywidgets 7.7.2 to make sure it's backward compatible, and I see no issues!

Comment on lines +948 to +953
var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}
var that = this;
if (msg.type === "before-attach") {
window.addEventListener("resize", function () {
that.autosizeFigure();
});
}

I guess we can remove this?

Copy link
Contributor Author

@abdelq abdelq Jan 9, 2023

Choose a reason for hiding this comment

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

It's been awhile so unsure, but if we want compatibility, probably not? This is not done in processLuminoMessage, just in processPhosphorMessage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the same code in _processLuminoMessage right? (Modulo the code changes due to #3805)

Copy link
Contributor Author

@abdelq abdelq Jan 9, 2023

Choose a reason for hiding this comment

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

Ah, this was from @nicolaskruchten's merge. Should perhaps be removed from _processLuminoMessage... I remember there were changes to resizing events and whatnot in newer ipywidgets.

Copy link

@Alexboiboi Alexboiboi Jan 10, 2023

Choose a reason for hiding this comment

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

Sorry to ask, but is this also related to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to jupyter-widgets/ipywidgets#3124, but I think with this current PR shrinking actually works fine (worth double checking).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's please remove those lines, as they are indeed already executed in the this._processLuminoMessage call immediately above

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise we get two event listeners, right?

Choose a reason for hiding this comment

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

The shrinking works fine for me too ;)

But inside an ipywidgets container it stilll does not really resizes correctly. Maybe what @maartenbreddels suggested, applying the [bqplot/bqplot#1531] strategy would solve the issues.

@maartenbreddels
Copy link
Contributor

I don't wanna drag this PR too long, but for the future, using a strategy that @mariobuikhuizen used in bqplot/bqplot#1531 we can get rid of the lumini/phosphor 'callbacks'.

@nicolaskruchten nicolaskruchten merged commit 5afac75 into plotly:master Jan 10, 2023
@nicolaskruchten
Copy link
Contributor

OK, thanks everyone, especially @abdelq ! This'll get into the next release later this week :)

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.

8 participants