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
Carriage return perf #6304
Carriage return perf #6304
Conversation
characters so we don't have to remove them over and over again.
private namespace.
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
|
||
// If we are given a IModelDB, keep an up-to-date | ||
// serialized copy of the OutputAreaModel in it. | ||
if (options.modelDB) { |
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.
Should we also delete the modelDB option in the options interface?
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.
Sure, I guess that's a breaking change, but one that I am pretty sure was used nowhere but in the context of Google RTC.
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.
Can you note it above in the breaking changes section?
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.
oh, you already did. Thanks!
Nice, the progress bar example in #4202 is now really smooth! |
Looks good to me. |
Thanks! Let's merge after tests pass. |
Looks like the linux integrity test failed because a timeout to yarn? I tried restarting it, but it seems to ignore my efforts to rerun. |
738653f
to
e7774e9
Compare
Not sure if it was the cause of the timeout, but there was a real mistake I just fixed. Let's see if that changes things. |
Looks like the linux python test is having an error installing:
|
Maybe |
Or the linux images, since it seems like Windows was fine. |
Will the |
Thanks for the efforts on this one--cheering you on from the sidelines (as I'm sure others are too). REALLY looking forward to being able to reliably do long running deep learning training runs in JupyterLab. |
@vidartf that's a good point. My hope had still been to remove it entirely (and this particular application of it has always smelled off), but if you feel it would still be useful as a bridge, we can restore it here. |
I'm going to backport that first commit and make a 0.35.x release tomorrow. Thanks @ian-r-rose, this one was a 🐻! |
Awesome, thanks @blink1073. Note that the second commit contains the fix. |
👍 |
Fixes #4202, for real this time. Boy, it's kind of tricky to debug something for which the test case locks up the browser. The problem was essentially a bad cached value. We were overwriting carriage returns and backspaces correctly, but then not updating the cached value of the overwritten output stream. This meant that we were doing the same work, over, and over, and over, in a quadratically growing fashion.
The real fix for this is a one-liner in b899ab0 . The rest of this PR is just some cleanup.
References
#4202.
Code changes
Restructuring of private functionality.
User-facing changes
None, except that the browser won't lock up when using progress bars.
Backwards-incompatible changes
Removal of
IModelDB
from output area input options. So far as I know, this was only used by the now-broken Google RTC.