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

stderr not displayed if using a non-interactive terminal #684

Closed
jlewi opened this issue Oct 14, 2024 · 8 comments
Closed

stderr not displayed if using a non-interactive terminal #684

jlewi opened this issue Oct 14, 2024 · 8 comments
Assignees

Comments

@jlewi
Copy link
Contributor

jlewi commented Oct 14, 2024

As a workaround for jlewi/foyle#286 I started configuring code cells to run in a non-interactive terminal. When I do this, I notice that errors (I think stderror) don't show up.

Here's a video. In this video I'm running a gcloud command that should return an error because the command line argument is incorrect.

runme-no-error-non-interactive.mp4

I'm pretty sure gcloud is writing errors to standard error and not stdout because we can do the following to redirect the error in this case to a file.

gcloud logging read "logName=projects/foyle-dev/logs/foyle AND jsonPayload.experiment=\"20241013-ghostmarkup\"" --limit=10 --age=24h 2>/tmp/error.log

Here's the notebook
https://gist.github.com/jlewi/d339af225bb4de94ca0845c342c421e0

If I explicitly redirect stderr to stdout as in the command below then the error will show up in the notebook.

gcloud logging read "logName=projects/foyle-dev/logs/foyle AND jsonPayload.experiment=\"20241013-ghostmarkup\"" --limit=10 --age=24h 2>&1

Is this working as intended? As a user, my expectation would be that stdout and stderr would both show up by default just as they would in a terminal.

For Foyle, its important that we capture stderr because we'd like the AI to make suggestions to fix the error. Since a cell can have multiple output-items one option would be to store stdout and stderr as separate items. Foyle could then figure out how to process them.

@jlewi
Copy link
Contributor Author

jlewi commented Oct 17, 2024

Related:
jlewi/foyle#286 (comment)

Here is @sourishkrout 's comment

Re, stderr in non-interactive, I've not been happy with how we're handling stdout vs stderr. I have a refactor on my list that will only send stdout into the notebook (primarily because third-party rich renderers are stdio-unaware) and, unless otherwise configured on the cell or doc, will omit stderr in the notebook-integrated terminal. However, with the same refactor, we'll introduce a multiplexed panel-based terminal that will receive both stdout and stderr. The idea is to enable the UI/UX to highlight the panel-based terminal for non-zero exits, either auto-focusing the terminal or providing a button to call the user's attention to the error.

@sourishkrout what's a panel here? Are you referring to how an outputitem gets rendered in the vscode notebook?
Do you mean that to handle non-zero exits

  • You will create an OutputItem that has stdout & stderr
  • A new mimetype will be introduced to trigger a custom renderer?

How does this compare to the following

a) Use a single OutputItem with mimetype application/vnd.code.notebook.stdout but the contents are actually stdout & stderr
b) Use two separate Outputs each with one OutputItem with mime type application/vnd.code.notebook.stdout but one of the items is stdout and the other is stderr?

@sourishkrout
Copy link
Member

sourishkrout commented Oct 26, 2024

@sourishkrout what's a panel here? Are you referring to how an outputitem gets rendered in the vscode notebook? Do you mean that to handle non-zero exits

When I say "panel" I mean the vscode-integrated terminal in the bottom panels (see screenshot below) where we'are already multiplexing interactive cell's stdout/stderr. The current impl forgoes this for non-interactive because we didn't think we needed it then.

image
  • You will create an OutputItem that has stdout & stderr
  • A new mimetype will be introduced to trigger a custom renderer?

The solution I'm favoring (still open) is to have the 1.) OutputItem only receive stdout by default (unless changed in a new cell-level annotation to receive both) and 2.) send both stdout/stderr to the vscode-terminal in the bottom panel. It'd be trivial to auto-focus the vscode-terminal upon non-zero exit code.

How does this compare to the following

a) Use a single OutputItem with mimetype application/vnd.code.notebook.stdout but the contents are actually stdout & stderr b) Use two separate Outputs each with one OutputItem with mime type application/vnd.code.notebook.stdout but one of the items is stdout and the other is stderr?

While not impossible, I believe it might be difficult to follow unless you have more than average knowledge about stdio. However, both approaches aren't mutually exclusive and could be combined.

@sourishkrout
Copy link
Member

Case-in-point for notebook/vscode terminal multiplexing is that interactive cells, if you focus/open the integrated terminals, will currently auto-close the bottom panel upon successful completion. This is due to closeTerminalOnSuccess (https://docs.runme.dev/configuration/cell-level#cell-configuration-keys) defaulting to true. For non-interactive it'd be the inteverse, open/focus the terminal on non-zero exit.

@jlewi
Copy link
Contributor Author

jlewi commented Oct 26, 2024

Let me see if I understand correctly.

  • Interactive terminals multiplex stdout and stderr and show the multiplexed stream in the Cell output window
  • non-interactive terminals don't multiplex stdout/stderr and only stdout is sent to the cell output window
  • non-interactive terminals don't multiple stdout & stderr because this could interfere with 3rd party renders
    • For example imagine a CLI is emitting a JSON table to stdout and also sending errors to stderr
    • If you multiple stdout & stderr you no longer have a stream of valid JSON
    • So a 3rd party renderer for JSON would break if you multiplexed stdout and stderr
  • So the proposed fix for non-interactive terminals is to send stderr to the vscode integrated terminal in the bottom panel

I don't think I appreciated all the subtleties here; thank you for the detailed explanation.

I don't love the idea of having to look at the vscode integrated terminal in the bottom panel to see stderr for non-interactive cells. Apriori, my expectation for notebooks is that errors should appear in the cell outputs (i.e. that's how it would work if I ran it jupyter or vscode with ipython kernel).

One takeway from this discussion is that for Foyle, I should go back to returning interactive cells by default so that stderr shows up by default. Users can then explicitly switch to non-interactive for other renderers. This also means I need to ensure completions are properly triggered for interactive cells.

@sourishkrout
Copy link
Member

Let me see if I understand correctly.

  • Interactive terminals multiplex stdout and stderr and show the multiplexed stream in the Cell output window

  • non-interactive terminals don't multiplex stdout/stderr and only stdout is sent to the cell output window

  • non-interactive terminals don't multiple stdout & stderr because this could interfere with 3rd party renders

    • For example imagine a CLI is emitting a JSON table to stdout and also sending errors to stderr
    • If you multiple stdout & stderr you no longer have a stream of valid JSON
    • So a 3rd party renderer for JSON would break if you multiplexed stdout and stderr
  • So the proposed fix for non-interactive terminals is to send stderr to the vscode integrated terminal in the bottom panel

Correct. This is how I propose the notebook terminals should work by default. The existing implementation is more obscure and unintentional, and I've been unhappy with it for a while now.

I don't think I appreciated all the subtleties here; thank you for the detailed explanation.

No sweat. Unless you really "have to care," one only sees the tip of the iceberg, that's stdio + char devices.

I don't love the idea of having to look at the vscode integrated terminal in the bottom panel to see stderr for non-interactive cells. Apriori, my expectation for notebooks is that errors should appear in the cell outputs (i.e. that's how it would work if I ran it jupyter or vscode with ipython kernel).

Ultimately, we likely want both, errors in the integrated and notebook terminals. The latter involves a non-trivial amount of UX/UI work, which I'd like to defer in favor of a short-term fix that's low-hanging fruit. I am currently working on a branch to get this done.

One takeway from this discussion is that for Foyle, I should go back to returning interactive cells by default so that stderr shows up by default. Users can then explicitly switch to non-interactive for other renderers. This also means I need to ensure completions are properly triggered for interactive cells.

I'd strongly suggest always defaulting to interactive, especially for shell scripts. Even simple commands aren't always TTY-aware and might block execution awaiting user input. For better non-interactive UX, the notebook cell should offer a button when applicable to rerun the cell switched to non-interactive when it detects a non-plain-text media type.

@sourishkrout
Copy link
Member

I got it roughly working below, @jlewi. Since we benefit from a PTY/TTY, I'm emitting stderr in red coloring.

Image

@sourishkrout
Copy link
Member

There is some cleanup to do, but this matches above's summary:

Stdio.mp4

@sourishkrout
Copy link
Member

This is fixed as per stateful/vscode-runme#1763, and the follow-up issue was created stateful/vscode-runme#1831.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants