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

include stream output in CellExecutionError #282

Merged
merged 4 commits into from Apr 19, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Apr 14, 2023

stream output may be useful for diagnosis, and does not appear to be captured for review elsewhere. I recently encountered a tricky to debug error because of a doubly-displayed error where the 'real' error is displayed earlier, while the terminating exception referred to prior output.

Example output:

An error occurred while executing the following cell:
------------------
import sys
print("hello")
print("errorred", file=sys.stderr)
# üñîçø∂é
raise Exception("message")
------------------

----- stdout -----
hello
----- stderr -----
errorred
------------------

---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
Cell In[1], line 5
      3 print("errorred", file=sys.stderr)
      4 # üñîçø∂é
----> 5 raise Exception("message")

Exception: message
Exception: message

Things to consider:

  • just concatenate streams instead of using stream labels? This would be more concise, cleaner, but lose some information
  • add an option, and make including stream output opt-out or opt-in. Could even be per-channel.
  • (configurable?) truncation limit, since stream output can be arbitrarily large

@minrk minrk added the enhancement New feature or request label Apr 14, 2023
stream output may be useful for diagnosis,
and does not appear to be captured for review elsewhere
@davidbrochart
Copy link
Member

Thanks @minrk, it looks like the test failures are relevant?

@minrk
Copy link
Member Author

minrk commented Apr 17, 2023

Yes, I think they are. I'm trying to figure out exactly what Windows does differently re: unicode and how to account for it. The behavior looks correct, it's just the test to actually check for unicode output (not affected or added by this PR, but never tested before because it was within a pytest.raises context after the exception, so never checked).

@minrk
Copy link
Member Author

minrk commented Apr 17, 2023

Actually, on further debugging it seems the unicode behavior is not correct on Windows (perhaps a wrong encoding somewhere?). Since that's not added or changed by this PR, is it okay to skip just that check, and restore it in a dedicated issue for Windows unicode issues?

skip it for now, because it's not part of this PR
@davidbrochart
Copy link
Member

is it okay to skip just that check, and restore it in a dedicated issue for Windows unicode issues?

Sounds good.

Things to consider:

  • just concatenate streams instead of using stream labels? This would be more concise, cleaner, but lose some information
  • add an option, and make including stream output opt-out or opt-in. Could even be per-channel.
  • (configurable?) truncation limit, since stream output can be arbitrarily large

Interesting ideas to keep in mind, maybe in other PRs?

@minrk
Copy link
Member Author

minrk commented Apr 17, 2023

Interesting ideas to keep in mind, maybe in other PRs?

I'm okay with this PR as-is if others are. Those were just the unaddressed ideas I had so far.

@davidbrochart davidbrochart enabled auto-merge (squash) April 19, 2023 07:35
@davidbrochart
Copy link
Member

It looks like tests have to be updated?

@davidbrochart davidbrochart merged commit 314c12e into jupyter:main Apr 19, 2023
23 of 25 checks passed
@davidbrochart
Copy link
Member

Thanks @minrk !

@minrk minrk deleted the show-out-err branch April 19, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants