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

Progressively render the template using jinja's generate method #133

Merged

Conversation

maartenbreddels
Copy link
Member

@maartenbreddels maartenbreddels commented May 13, 2019

If you call Template.generate instead of Template.render, jinja returns snippets (for each statement a snippet, not expressions!).
The key here is that:

{%- set nb = notebook_execute() -%}

Is a statement, so all html before this will be send to the browser (and allows the browser to progressively load, and show a spinner). However, it seems that the nb variable is not updates, the rest of the template seems to use the old 'nb' variable.

Replaces #71

@maartenbreddels maartenbreddels force-pushed the progressive_loading branch 3 times, most recently from e6745d4 to 29f2f8a Compare May 17, 2019 12:53
@maartenbreddels
Copy link
Member Author

There is a bit more control now, starting the kernel and executing the notebook are two steps, allowing for even quicker page loading. The flow of data is now:

  • everything from the start of the template until the next jinja expression is send to the client (can include a spinner, resources etc)
  • The kernel is started, and also this snippet is send to the client (we could for instance start making the websocket connection in the future).
{%- with kernel_id = kernel_start() -%}
  <script id="jupyter-config-data" type="application/json">
  {
      "baseUrl": "{{resources.base_url}}",
      "kernelId": "{{kernel_id}}"
  }
  </script>
.....
{% endwith %}

  • The cells are executed, and once they are done the rest of the template is send to the client.
  {# from this point on, nb.cells contains output of the executed cells #}
  {% do notebook_execute(nb, kernel_id) %}
    <div tabindex="-1" id="notebook" class="border-box-sizing">
      <div class="container" id="notebook-container">
        {{ super() }}
      </div>
    </div>

@maartenbreddels maartenbreddels changed the title WIP: Progressively render the template using jinja's generate method Progressively render the template using jinja's generate method May 17, 2019
@vidartf
Copy link
Contributor

vidartf commented Aug 8, 2019

Would build_widgets need to be called multiple times on the front end to have progressive rendering of widgets?

@maartenbreddels
Copy link
Member Author

Good point, that part needs refactoring, I think it would be good to separate that in a different PR. The 'issue' is that we now have two communication channels (http and websocket) that are async, so you can imagine the html outputting 'render widget with id XYZ', while that widget is not yet constructed via the websocket, and visa versa.

In voila-vuetify we had a similar issue (https://github.com/QuantStack/voila-vuetify/blob/7af86f0261f10947da33dc13f4e5a7a750c598c1/share/jupyter/voila/templates/vuetify-base/nbconvert_templates/util.js#L49), and if I'm not mistaken you've worked on a similar issue in the LabManager.

@vidartf
Copy link
Contributor

vidartf commented Aug 8, 2019

If the widget data has not arrived yet, the widget renderer can await the model promise. There should be support for this in the widget manager, so I think that is tractable.

The main reason I asked is that if we are continuously trying to check for <script> tags while streaming content, you run the risk that the script tag is incomplete, and that you therefore fail in an undefined manner. The only way I can think of that could work is to have build_widgets be global, and add a JS script tag at the end of every widget output that calls that function. And I'm not sure if I like that solution.

@vidartf
Copy link
Contributor

vidartf commented Aug 8, 2019

Hmm, maybe a better solution is MutationObserver ? I'll need to research that, including how it interacts with progressive rendering.

@vidartf
Copy link
Contributor

vidartf commented Aug 8, 2019

Actually, never mind the noise. Tornado correctly does chunking with the flush method.

@maartenbreddels
Copy link
Member Author

This is the result from this PR:
screencapture-progressive-cell

I've decided not to show the cell output until it's done, since we do not have progressive widget rendering yet. I think it keeps this PR at minimal and clean as possible.
I'll add some more comments on how things work, and why they work, and what the current limitation are with jinja2.

voila/handler.py Outdated
all_cells = list(nb.cells) # copy the cells, since we will modify in place
for cell in all_cells:
# we execute one cell at a time
nb.cells = [cell] # reuse the same notebook
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean we can't make a single copy of notebook, and use that copy to execute one cell at a time (nb_copy.cells = [cell]), while keeping the original notebook intact?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could, but as the code is now, there is no need for it. The general strategy in nbconvert is to do copy.deepcopy. But as of now, each requests reads the notebook file and gives a new notebook object.

@jtpio
Copy link
Member

jtpio commented Aug 13, 2019

Do we want the progressive rendering to be part of the default template, or move it to a new template?

@maartenbreddels
Copy link
Member Author

I think it should be a core feature of voila templates, together with the template refactor, I think we can have support for this in all templates with zero (yes free / out of the box) of minimal effort.

@jtpio
Copy link
Member

jtpio commented Aug 20, 2019

I think we can have support for this in all templates with zero (yes free / out of the box) of minimal effort.

That would be great, so template authors don't need to think about explicitly calling cell_generator when extending the default template.

@maartenbreddels
Copy link
Member Author

Yes, that's indeed the plan.

@jtpio
Copy link
Member

jtpio commented Aug 26, 2019

A few glitches with Firefox that we also noticed with @maartenbreddels on Friday:

  • The font awesome icon is not loaded
  • The spinner is not shown when refreshing the page

progressive-rendering

@jtpio jtpio mentioned this pull request Aug 27, 2019
@maartenbreddels
Copy link
Member Author

A few glitches with Firefox that we also noticed with @maartenbreddels on Friday:

My experimental branch with asyncio works perfect, I still see that it takes a while for Firefox to start rendering, but it does work.

@jtpio
Copy link
Member

jtpio commented Aug 28, 2019

My experimental branch with asyncio works perfect, I still see that it takes a while for Firefox to start rendering, but it does work.

Including the rendering of the font awesome icon?

@jtpio
Copy link
Member

jtpio commented Aug 28, 2019

I guess we could leave this for a follow-up PR (with the async goodness)

@maartenbreddels
Copy link
Member Author

Including the rendering of the font awesome icon?

Yes

@maartenbreddels maartenbreddels merged commit 9bdb023 into voila-dashboards:master Aug 30, 2019
@SylvainCorlay
Copy link
Member

🎉

@jtpio
Copy link
Member

jtpio commented Aug 30, 2019

Thanks @maartenbreddels!

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.

None yet

4 participants