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

ported the full output optimization code from VSCode #13137

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Dec 1, 2023

What it does

Fixes #13069
Fixes #13152
this ports all the ouput optimization vscode is doing over to theia.

How to test

See linked Task for specific test case.
Open or create a notebook and execute a cell with stdout output

Follow-ups

Review checklist

Reminder for reviewers

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

@JonasHelming Is there a specific commit we are allowed to use from vscode, so we don't have to create an IP check ticket? I remember I heard something about that in one of the dev calls.

packages/notebook/src/browser/notebook-output-utils.ts Outdated Show resolved Hide resolved
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden
Copy link
Contributor Author

jonah-iden commented Dec 1, 2023

Btw could any of you test this. It seems like there is still an issue because i somtimes get no stdout ouput at all.
im not able to start theia with python and jupyther installed right now. No idea why it seems its crashing when [ms-python.vscode-pylance]: trying to resolve latest version but im not getting any errors even when i put that code in a try catch

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@bvenreply
Copy link
Contributor

Hello @jonah-iden, I tested the PR on the minimal example I gave in the issue and now the output matches what is expected.

I also checked the original usecase that prompted the creation of the issue in the first place; Unfortunately, there's still a problem there:

I noticed that somehow, if both stdout and stderr streams are used in the output, the characters are no longer correctly erased. This does not occur in vscode.

You should be able to reproduce this with a small change to the Python code I provided in the issue:

import time
import sys

msg = "abcdef"
print("spoiler!", flush=True, file=sys.stderr)

for i in range(5):
    print(msg, end="", flush=True)
    time.sleep(1)
    for j in msg:
        print("\b", end="", flush=True)
    time.sleep(0.5)

This does not look strictly related to #13069 so if you feel I should open another issue let me know.

@jonah-iden
Copy link
Contributor Author

@bvenreply I'll take a look today and if i can't find problem quickly lets create a new Issue for that

@jonah-iden
Copy link
Contributor Author

Ok after a first look this new problem seems to be a larger issue inside the ouput update logic. I think we should create a separate issue for it. Maybe i can find some time on friday to take a deeper look

@jonah-iden jonah-iden marked this pull request as ready for review December 5, 2023 08:32
@bvenreply
Copy link
Contributor

Ok after a first look this new problem seems to be a larger issue inside the ouput update logic. I think we should create a separate issue for it. Maybe i can find some time on friday to take a deeper look

Got it. I will open it myself later today.

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me 👍

I can confirm that both issues are fixed with this change.

@JonasHelming
Copy link
Contributor

@bvenreply Could you confirm it works for you, please?

@jonah-iden jonah-iden merged commit 469bd74 into master Dec 8, 2023
14 checks passed
@github-actions github-actions bot added this to the 1.45.0 milestone Dec 8, 2023
@jonah-iden
Copy link
Contributor Author

@JonasHelming was allready confirmed on the issue #13152 (comment)

@JonasHelming
Copy link
Contributor

@JonasHelming was allready confirmed on the issue #13152 (comment)

perfect thank you, I missed that. Thanks for solving this so quickly and thanks for the original report and restesting!

@msujew msujew deleted the jiden/notebook-add-ouput-optimization branch December 8, 2023 14:46
@msujew msujew added the notebook issues related to notebooks label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks
Projects
Archived in project
6 participants