Skip to content

V1.10.0 - Inject plotly.js into the output cell on every init_notebook_mode call #469

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

Merged
merged 3 commits into from
May 20, 2016

Conversation

chriddyp
Copy link
Member

@chriddyp chriddyp commented May 19, 2016

No description provided.

chriddyp added 3 commits May 19, 2016 13:40
Fixes #458

Running init_notebook_mode injects plotly.js into the output cell.
Before, if you ran it twice, it would clear the output cell on the
second call and remove plotly.js from DOM.
This was deceiving, because any existing plots on the page wouldn’t
disappear even though their underlying plotly.js was no longer present.
But, if you refresh the page, the plots would not render (as plotly.js
was no longer in the output cell)
@chriddyp
Copy link
Member Author

chriddyp commented May 19, 2016

Full description of changes (from changelog):

Version 1.9.13 fixed an issue in offline mode where if you ran init_notebook_mode
more than once the function would skip importing (because it saw that it had
already imported the library) but then accidentally clear plotly.js from the DOM.
This meant that if you ran init_notebook_mode more than once, your graphs would
not appear when you refreshed the page.
Version 1.9.13 solved this issue by injecting plotly.js with every single iplot call.
While this works, it also injects the library excessively, causing notebooks
to have multiple versions of plotly.js inline in the DOM, potentially making
notebooks with many iplot calls very large.
Version 1.10.0 brings back the requirement to call init_notebook_mode before
making an iplot call. It makes init_notebook_mode idempotent: you can call
it multiple times without worrying about losing your plots on refresh.

@chriddyp chriddyp changed the title Inject plotly.js into the output cell on every init_notebook_mode call V1.10.0 - Inject plotly.js into the output cell on every init_notebook_mode call May 19, 2016
@chriddyp
Copy link
Member Author

chriddyp commented May 19, 2016

cc @yankev @theengineear @mdtusz @BRONSOLO @Kully @aggFTW

This fixes #458 in a slightly different way than version 1.9.13 proposed. This change fixes an issue in 1.9.13 introduced where plotly.js is injected into the DOM excessively.

This is the notebook that I am using for QA. Unfortunately, this stuff is still hard to catch in our automated tests. https://gist.github.com/chriddyp/170f46b36a9a3be7caee96eb3d4118bc - @yankev - could you make sure that this works as intended? @mdtusz - can we add a test that ensures that plotly.js isn't injected multiple times?

Thank you @aggFTW again for bringing #458 to our attention!

if not _ipython_imported:
raise ImportError('`iplot` can only run inside an IPython Notebook.')

global __PLOTLY_OFFLINE_INITIALIZED
# Inject plotly.js into the output cell
Copy link

Choose a reason for hiding this comment

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

You are missing

if not __PLOTLY_OFFLINE_INITIALIZED:

no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually, that's the heart of the fix. Before, we had this check:

if not __PLOTLY_OFFLINE_INITIALIZED:
    return

which meant that if you ran this function twice, we return None the second time which cleared the output cell (which previously had plotly.js in it). Now, we just inject plotly.js into the output cell every time.

Copy link
Contributor

@theengineear theengineear May 19, 2016

Choose a reason for hiding this comment

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

*__* oh, sweet! sorry, def confused by that on the first read.

@theengineear
Copy link
Contributor

💃 for the change after we hear back from @yankev on #469 (comment)

@aggFTW
Copy link

aggFTW commented May 19, 2016

What we do is we use plotly inside of an Output ipywidget. Can you check in your notebook that this still works after this change? Widgets are not persisted to the output fields.

Basically, add this to your test notebook code as a use case:

from ipywidgets import Output
from IPython.core.display import display
from plotly.offline import init_notebook_mode
from plotly.graph_objs import Pie, Figure, Data
from plotly.offline import iplot

o = Output()
display(o)

with o:
    print("I'm not persisted to output area!")
    init_notebook_mode()
    data = [Pie(values=[1,2,3])]
    fig = Figure(data=Data(data))
    iplot(fig, show_link=False)

@yankev
Copy link
Contributor

yankev commented May 19, 2016

@aggFTW so it seems that the Output widget persists, but the text and the pie chart don't after a refresh. Were the text and charts persisting in a previous version of plotly.py?

@aggFTW
Copy link

aggFTW commented May 19, 2016

No, it has never persisted. When plotly was broken, a refresh would show an exception though. We should avoid that behavior.

@chriddyp
Copy link
Member Author

@yankev sounds like we're good. can you fix or update the tests and deploy?

@yankev
Copy link
Contributor

yankev commented May 20, 2016

@chriddyp yep got it

@yankev yankev merged commit eaa2169 into master May 20, 2016
@nicolaskruchten nicolaskruchten deleted the offline-fixes branch June 19, 2020 16:13
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