-
Notifications
You must be signed in to change notification settings - Fork 502
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
Speed up progressive rendering #396
Conversation
@maartenbreddels what do you think? It improves perfomances but the logic was also wrong before (processing each cell like it's a single Notebook) |
Part of the slowness was coming from the fact that internally nbconvert creates new sockets for connecting with the kernel at each |
Reviewed this in person with @martinRenou and this looks great. |
I'm wondering if the |
I now reimplement |
There was a problem hiding this 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).?
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Should fix #395
Instead of using the
preprocess
method (which is for preprocessing an entire Notebook) for each cell, I usepreprocess_cell
for each cell