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

Speed up progressive rendering #396

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Oct 3, 2019

Should fix #395

Instead of using the preprocess method (which is for preprocessing an entire Notebook) for each cell, I use preprocess_cell for each cell

speedup

@martinRenou martinRenou changed the title Speed up progressive rendering WIP - Speed up progressive rendering Oct 3, 2019
@martinRenou
Copy link
Member Author

@maartenbreddels what do you think? It improves perfomances but the logic was also wrong before (processing each cell like it's a single Notebook)

@martinRenou
Copy link
Member Author

Part of the slowness was coming from the fact that internally nbconvert creates new sockets for connecting with the kernel at each preprocess call (that is to say for each cell...)

@martinRenou martinRenou changed the title WIP - Speed up progressive rendering Speed up progressive rendering Oct 3, 2019
@SylvainCorlay
Copy link
Member

Reviewed this in person with @martinRenou and this looks great.

@martinRenou martinRenou changed the title Speed up progressive rendering WIP - Speed up progressive rendering Oct 3, 2019
@martinRenou
Copy link
Member Author

I'm wondering if the preprocess implemented in the Voila executor has extra logic that we should have. Maybe the Voila executor should reimplement preprocess_cell

@martinRenou
Copy link
Member Author

martinRenou commented Oct 3, 2019

I now reimplement preprocess_cell in the Voila executor so that it has a similar behavior to preprocess concerning error handling

@martinRenou martinRenou changed the title WIP - Speed up progressive rendering Speed up progressive rendering Oct 3, 2019
Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Pretty nice! I see the same performance increase in the asyncio PR #374, but lets get this in first, it is much simpler. Then we make in multithreaded and after that worry about true asyncio.

Would you mind adding a unittest that tests that the output is removed (since I think we can consider this a security risk if it contains a stacktrace).?

voila/handler.py Show resolved Hide resolved
Comment on lines +111 to +117
try:
result = super(VoilaExecutePreprocessor, self).preprocess_cell(cell, resources, cell_index, store_history)
except CellExecutionError as e:
self.log.error(e)
result = [cell]
return result

Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

To mimic the VoilaExecutePreprocessor.preprocess behavior. I change this behavior a bit in a separate PR concerning error outputs BTW.

Copy link
Member Author

Choose a reason for hiding this comment

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

The try except prevents a 505 server error when the Notebook contains an error

@maartenbreddels maartenbreddels merged commit c6dc53e into voila-dashboards:master Oct 7, 2019
@martinRenou martinRenou deleted the speedup_progessive_rendering branch October 7, 2019 15: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.

Progressive rendering is super slow
3 participants