stateful / runme

DevOps Notebooks Built with Markdown
https://runme.dev
Apache License 2.0
1.2k stars 38 forks source link

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

Open jlewi opened 1 month ago

jlewi commented 1 month ago

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.

https://github.com/user-attachments/assets/6b81e50f-5f00-495f-aad3-bea95d26e8b7

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 commented 1 month ago

Related: https://github.com/jlewi/foyle/issues/286#issuecomment-2414499139

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

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

Let me see if I understand correctly.

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 commented 3 weeks ago

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 commented 3 weeks ago

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

Image

sourishkrout commented 3 weeks ago

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

https://github.com/user-attachments/assets/ddd5a628-9e2b-4c8e-bce7-315619219082