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

Only show the head of the outputs and ensure iopub outputs are correctly displayed #11457

Merged
merged 6 commits into from Nov 19, 2021

Conversation

echarles
Copy link
Member

@echarles echarles commented Nov 14, 2021

References

Fixes #10045

Code changes

We need to take into account the iopub messages when stripping the output to only show the defined max number of outputs.

This PR takes those messages into account. To achieve this, we also had to remove the display of the tail.

User-facing changes

The outputs are correctly displayed when running e.g.

for i in range(100):
    for x in range(10):
        print(x, flush=True)
    for x in range(10):
        print(x, file=sys.stderr, flush=True)

the tail is not more displayed (all outputs can still be seen if the user clicks on the message Click on this message to get the complete output.)

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @echarles I let some minor suggestions otherwise it works as expected.

packages/notebook-extension/schema/tracker.json Outdated Show resolved Hide resolved
packages/outputarea/src/widget.ts Outdated Show resolved Hide resolved
packages/outputarea/src/widget.ts Outdated Show resolved Hide resolved
packages/outputarea/src/widget.ts Outdated Show resolved Hide resolved
echarles and others added 4 commits November 16, 2021 16:31
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
…n outputs is clickable

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@echarles
Copy link
Member Author

Thx a lot @fcollonval for the review. I have committed your suggestions.

@krassowski krassowski added the bug label Nov 16, 2021
@jtpio jtpio added this to the 3.2.x milestone Nov 16, 2021
@jtpio
Copy link
Member

jtpio commented Nov 16, 2021

This needs a quick lint pass.

@echarles
Copy link
Member Author

@fcollonval @jtpio The code in now linted and CI is green. This is ready for merge.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

All green for me. Thanks @echarles

@github-actions
Copy link
Contributor

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table
Test file large_code_notebook large_md_notebook
open
chromium
actual 7167 <- [7540 - 7867 - 7965] -> 8239 3500 <- [3587 - 3643 - 3906] -> 4047
expected 6437 <- [7494 - 7828 - 7971] -> 8240 3496 <- [3590 - 3627 - 3724] -> 4174
Mean relative change 0.4% ± 1.1% 1.0% ± 1.3%
switch-from
chromium
actual 842 <- [912 - 965 - 1153] -> 1633 559 <- [611 - 826 - 863] -> 957
expected 826 <- [893 - 942 - 1148] -> 1581 559 <- [618 - 820 - 898] -> 951
Mean relative change 2.0% ± 4.3% -0.7% ± 4.7%
switch-to
chromium
actual 947 <- [986 - 1006 - 1021] -> 1255 725 <- [832 - 851 - 864] -> 902
expected 943 <- [979 - 993 - 1008] -> 1112 713 <- [815 - 838 - 856] -> 902
Mean relative change 1.6% ± 1.0% 1.8% ± 1.6%
close
chromium
actual 997 <- [1074 - 1105 - 1460] -> 1593 670 <- [696 - 705 - 717] -> 853
expected 965 <- [1061 - 1109 - 1179] -> 1581 663 <- [691 - 703 - 713] -> 821
Mean relative change 2.9% ± 4.4% 0.6% ± 1.1%

Changes are computed with expected as reference.

@echarles
Copy link
Member Author

Thx for the review and approval @fcollonval Will you merge this?

@fcollonval
Copy link
Member

Thx for the review and approval @fcollonval Will you merge this?

Doing it now...

@fcollonval fcollonval merged commit d1c2613 into jupyterlab:master Nov 19, 2021
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.2.x

@lumberbot-app
Copy link

lumberbot-app bot commented Nov 19, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.2.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 d1c26138eaecfd21e388703e704215b345c1166c
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #11457: Only show the head of the outputs and ensure iopub outputs are correctly displayed'
  1. Push to a named branch:
git push YOURFORK 3.2.x:auto-backport-of-pr-11457-on-3.2.x
  1. Create a PR against branch 3.2.x, I would have named this PR:

"Backport PR #11457 on branch 3.2.x (Only show the head of the outputs and ensure iopub outputs are correctly displayed)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@echarles
Copy link
Member Author

Thx for the merge @fcollonval

I have seen a need for a manual backport to 3.2.x and a branch in your repo. I have seen tags removed by @krassowski

To avoid confusion, I have opened #11509 which backport this fix to 3.2.x branch.

@krassowski
Copy link
Member

I removed the tag because the backport PR was already opened by @fcollonval here: #11502 so your PR is a duplicate ;)

@echarles
Copy link
Member Author

I removed the tag because the backport PR was already opened by @fcollonval here: #11502 so your PR is a duplicate ;)

Yeah, I was suspecting that, but for some reasons, I have not found @fcollonval PR... I will close mine. Better 2 than zero :)

echarles added a commit that referenced this pull request Nov 20, 2021
…n-3.2.x

Backport PR #11457 on branch 3.2.x (Only show the head of the outputs and ensure iopub outputs are correctly displayed)
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug pkg:notebook pkg:outputarea status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

maxNumberOutputs bug
4 participants