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

first pass at nteract support #562

Merged
merged 13 commits into from
Feb 3, 2017
Merged

first pass at nteract support #562

merged 13 commits into from
Feb 3, 2017

Conversation

chriddyp
Copy link
Member

@chriddyp chriddyp commented Sep 2, 2016

No description provided.

@chriddyp
Copy link
Member Author

chriddyp commented Sep 2, 2016

Here's a first pass at offline interact support. Here's a comment in the commit for some details:

Display 2 versions of the figure:
 - The raw HTML is the "old-school" style - we just throw in a JS script
   inside the cell. This works for a lot of environments but some JS is
   stripped out in some rendering environments / platforms like
   GitHub, nteract, and eventually Plotly
 - The custom `mime-types` is the new-school way. nteract supports
   rendering plotly graphs this way (see https://github.com/nteract/nteract/pull/662)
   and others will hopefully follow suite.
 Both of these bundles will be saved as part of the notebook. The renderer
 will choose which one to render. So 'text/html' is like a fallback for
 any environment that doesn't have the 'application/json+plotly.v1'
 renderer available.
 The bummer is that we're injecting the figure JSON in here twice, so
 notebooks that are pretty big are about to get a lot bigger.
 The upside is that we're dropping any compatability for older notebook
 versions

@chriddyp
Copy link
Member Author

chriddyp commented Sep 2, 2016

Screenshot from nteract:
image

Associated nteract PR: nteract/nteract#662

@chriddyp
Copy link
Member Author

chriddyp commented Sep 2, 2016

Works across nteract and nbviewer, however opening these notebooks inside my existing jupyter notebook causes a validation error. It's harmless I think, but a pretty scary message to see.

image

@chriddyp chriddyp closed this Sep 2, 2016
@chriddyp chriddyp reopened this Sep 2, 2016
@chriddyp
Copy link
Member Author

chriddyp commented Sep 2, 2016

# Both of these bundles will be saved as part of the notebook. The renderer
# will choose which one to render. So 'text/html' is like a fallback for
# any environment that doesn't have the 'application/json+plotly.v1'
# renderer available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, yah, this makes sense to me. Agreed that notebooks getting bigger is a bummer... I'm not sure how much of an edge case that is.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

That's some hardcore validation. I'll bet if our key isn't application/json+ it would let it be.

screen shot 2016-09-02 at 2 33 28 pm

@minrk, @fperez does this fall out of spec?

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

Tried out application/vnd.plotly.v1+json in case it was just a hard check on format within application/json. No go. This is going to affect the custom mimetype for the widgets in JupyterLab too.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

Ah, here we go. Pretty explicit in the schema here if not the intention. If it's a custom mimetype it expects an array of strings or a string.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

I suppose we'd need the notebook schema to also have:

"patternProperties": {
  "^application/json\+[a-zA-Z0-9]+/[a-zA-Z0-9\\-\\+\\.]+$": {
    "type": "object"
  }
}

for JupyterLab/ipywidgets:

"patternProperties": {
  "^application/vnd\.[a-zA-Z0-9]+/[a-zA-Z0-9\\-\\+\\.]+$": {
    "type": "object"
  }
}

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

Current proposal for registering custom mimetypes in nteract: nteract/nteract#670

@fperez
Copy link

fperez commented Sep 2, 2016

Not quite sure where your thinking is right now, are you going in the direction of extending the nb schema?

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

I'd like to pick the right future direction while not crippling usage of the classic notebook. There are quite a few options here, so I was outlining some thoughts on them.

@fperez
Copy link

fperez commented Sep 2, 2016

Got it. I guess once this is a bit more fleshed out, would be good to run it by the Jupyter list for broader thoughts.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

Yeah, I'll make an outline in an email on the jupyter list. It's working quite nicely in nteract, and I'm working on vega support, soon bokeh as well.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 2, 2016

Beyond that though, it seems like the only way for us to do custom extensions is to make them be double encoded JSON in the current schema. Only the pure field application/json allows regular objects.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 3, 2016

Per resolution in jupyter/nbformat#43, the plotly mimetype should be application/vnd.plotly.v1+json.

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 6, 2016

Since jupyter/nbformat#42 has been merged we can now send over the raw JSON on the next nbformat release.

However, since users of old versions of the notebook should allow this to still work, we can send the serialized JSON as a string directly, which was handled for backwards compatibility in nteract here: https://github.com/nteract/nteract/pull/675/files#diff-18d3d49b31452599ff46a67c571aa4f8R7

One other thing that still needs to be changed is that the mimetype should now be application/vnd.plotly.v1+json

@theengineear
Copy link
Contributor

@chriddyp @rgbkrk just tried this and it seemed to work fine?

image

(this is just the current plotly release that I used)

Has something just changed in nteract?

@chriddyp
Copy link
Member Author

@theengineear - online mode works fine, this would just be for offline support.
I'm all for supporting this PR. the only drawback is that we will be including the bundle and the Json twice: once for the interact minetype and another for Jupiter notebooks that don't support the mimetype enhancement yet. while we could detect which notebook type the author is using, we don't know which notebook type the viewer might be using, so we need to include both. big notebooks will go from 15MB to 30MB.

it doesn't seem like there is any getting around this, so it seems like it's something that we should just add. thoughts?

@jackparmer
Copy link
Contributor

jackparmer commented Jan 18, 2017

+1 from me. We can't really promote nteract too much without offline mode working. We'd just get too many support Q's.

# The upside is that we're dropping any compatability for older notebook
# versions
display_bundle = {
'application/json+plotly.v1': {
Copy link
Contributor

Choose a reason for hiding this comment

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

@chriddyp I believe this needs to be updated now (as per #562 (comment)) to application/vnd.plotly.v1+json.

@theengineear
Copy link
Contributor

^^ weird, sign_in seemed to fail all over the place in our tests... rebuilding...

@theengineear
Copy link
Contributor

hrm... I think there's something off with how other tests are getting sign_in'd to. I.e., the setup for the tests is not sandboxed... which is problematic. I'm going to take the time to get this right.

We should have our tox (or CI?) setup always start the build off by deleting ~/.plotly/. Then, the more difficult change is to ensure that we use the PlotlyTestCase whenever we write a test that requires changing config/credentials in any way.

This will likely take a little while since a lot of our tests are still written in module-style (i.e., not class-based), so we need to first refact these tests so that we can properly subclass PlotlyTestCase 😿. Maw.

☕️ ☕️ ☕️ It's worth getting this correct now, we keep running into these issues.

Really, fewer of our tests should even be hitting a Plotly server, that that's a problem for another day...

Ideally, we'd just totally mock out the Python API and setup a suite of API tests inside the streambed repo. Again, definitely for another day, but it would greatly speed up our tests and completely decouple our tests from any specific plotly server.

@theengineear
Copy link
Contributor

@chriddyp @rgbkrk I got tests passing and AFAIK offline mode is working as expected:

image

Anything else to be done here? I'm a little booked up on other tasks and don't want to let this linger much longer.

//cc @jackparmer

@theengineear
Copy link
Contributor

^^ That's this nteract version

@theengineear
Copy link
Contributor

Sorry to keep pinging, but are we all good with this @chriddyp? Anything else you wanted to add?

@theengineear
Copy link
Contributor

It's got my 💃 :)

Copy link
Contributor

@rgbkrk rgbkrk left a comment

Choose a reason for hiding this comment

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

Cool to see this moving forward!

@@ -273,7 +273,7 @@ def iplot(figure_or_data, show_link=True, link_text='Export to plot.ly',
validate=True, image=None, filename='plot_image', image_width=800,
image_height=600):
"""
Draw plotly graphs inside an IPython notebook without
Draw plotly graphs inside an IPython or Jupter notebook without
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Jupter/Jupyter/

# - The raw HTML is the "old-school" style - we just throw in a JS script
# inside the cell. This works for a lot of environments but some JS is
# stripped out in some rendering environments / platforms like
# GitHub, nteract, and eventually Plotly
Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript is not stripped out in nteract - the core differences are that:

  • require is commonjs require rather than requirejs' require in the desktop app
  • we don't have the same JavaScript API as the Jupyter notebook

Neither of those should be relied on for HTML outputs regardless (or at least some primitive feature detection and fallbacks need to happen).

Copy link
Contributor

Choose a reason for hiding this comment

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

^^ I ended up just 🔪-ing a lot of that comment out, I don't think it needs to get committed here.

# GitHub, nteract, and eventually Plotly
# - The custom `mime-types` is the new-school way. nteract supports
# rendering plotly graphs this way (see https://github.com/nteract/nteract/pull/662)
# and others will hopefully follow suite.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/follow suite/follow suit/

@chriddyp
Copy link
Member Author

chriddyp commented Feb 3, 2017

looks great to me, this is awesome! 💃

@Madhu94
Copy link

Madhu94 commented Feb 5, 2018

while we could detect which notebook type the author is using, we don't know which notebook type the viewer might be using, so we need to include both. big notebooks will go from 15MB to 30MB.

@chriddyp Did you find a way around this ? Or, do notebooks with multiple mime types just have to be larger ?

@rgbkrk
Copy link
Contributor

rgbkrk commented Feb 5, 2018

They only have to be larger if you wish to support old versions of the notebooks.

@Madhu94
Copy link

Madhu94 commented Feb 5, 2018

Thanks for the quick response @rgbkrk. Sorry, rephrasing my question, "Notebooks with custom mime-types which also have to be used with nbviewer just have to be larger? "

I guess that is a question to be asked in the nbconvert/nbviewer repo.

@rgbkrk
Copy link
Contributor

rgbkrk commented Feb 5, 2018

Someone would have to add support to nbconvert and nbviewer, that's for sure.

@Madhu94
Copy link

Madhu94 commented Feb 5, 2018

I'll take a shot.

Ipywidgets render in nbviewer by fetching the widget models and views from unpkg, and i think even this can now be configured. Maybe a similar strategy for fetching mime-renderers from somewhere..?

Anyway, I'll open a ticket for discussion. Thanks a lot. :)

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.

6 participants