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

Load plotly.js from CDN rather than by inclusion #439

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

baldwint
Copy link

Greatly reduces notebook file size by setting connected=True in the call to init_notebook_mode, as suggested here. Fixes #438.

Greatly reduces notebook file size by setting `connected=True` in the call to `init_notebook_mode`, as suggested here: https://github.com/plotly/plotly.py/blob/master/plotly/offline/offline.py#L92 . Fixes jupyter-incubator#438.
@aggFTW
Copy link
Contributor

aggFTW commented Feb 13, 2018

Hey, thanks so much for sending this 👍

We have autovizwidget listing plotly version requirements here:
https://github.com/jupyter-incubator/sparkmagic/blob/master/autovizwidget/requirements.txt#L1
and here:
https://github.com/jupyter-incubator/sparkmagic/blob/master/autovizwidget/setup.py#L60

1.10.0 doesn't have that parameter available. It was introduced in 1.13.0. Could you please update the requirements there? I think it's good after that!

To support the connected=True argument to init_noteboook_mode.
@baldwint
Copy link
Author

Thank you for the review! And good catch. I modified the two files to update the minimum plotly version required.

@aggFTW
Copy link
Contributor

aggFTW commented Feb 16, 2018

LGTM. Waiting for @apetresc's feedback.

@apetresc
Copy link
Member

Did some manual testing and everything seems to work fine 😄

That being said, I think it would be optimal to allow this to be configurable in ~/.sparkmagic/config.json, maybe with a key like embed_plotly - and I agree that connected=True (i.e, embed_plotly=False) should be the default value.

The tradeoff being made is that if we force connected=True, exported notebooks won't work at all if opened on a laptop without an internet connection, or on a particularly restrictive firewall, with no way to fix it.

I can make this change, just wanted to get @aggFTW's approval (on both the idea and the name of the key in config.json).

@aggFTW
Copy link
Contributor

aggFTW commented Feb 21, 2018

Yes, that sounds good! 👍 Will you merge then?

@thesuperzapper
Copy link

@apetresc @aggFTW is this going to be merged?

@thesuperzapper
Copy link

@baldwint

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