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

Rewire stdio for non-interactive #1763

Merged
merged 5 commits into from
Dec 2, 2024
Merged

Conversation

sourishkrout
Copy link
Member

@sourishkrout sourishkrout commented Oct 29, 2024

As @jlewi summarized in stateful/runme#684:

  • 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

This PR aligns the user experience with the above. Todos:

  • Add/fix tests
  • Test dagger integration continues to work
  • Test cloud resource integration continues to work
  • Finalize stderr "color" if any, it's a less alarming terminal-yellow right now

Btw, this PR does not solve how to include stderr into the serialized notebook, as per:

and for non-interactive we could probably do:

  • stateful.runme/output-items
  • application/vnd.code.notebook.stdout
  • perhaps a third with application/vnd.code.notebook.stderr

We'll handle this in a separate PR (cc @jlewi).

@sourishkrout sourishkrout marked this pull request as draft October 29, 2024 16:49
@sourishkrout
Copy link
Member Author

@jlewi, here's a demo. You should be able to run it locally. Merging it still requires more work, though.

Stdio.mp4

@jlewi
Copy link
Contributor

jlewi commented Oct 30, 2024

The UX looks great. One question I have for non-interactive cells, if I wanted to programmatically access stderr in order to send it to Foyle, where would I tap into it.

Consider the following example

  1. We use gcloud to fetch logs from Cloud Logging
  2. We use a custom renderer to render logs in a rich way
  3. gcloud fails with an error because the query is wrong

In this case what I'd like to happen is

  1. We send a request to Foyle with the stderr containing the error message about the logging query
  2. Foyle returns a corrected command which is inserted as a ghost cell.

@sourishkrout
Copy link
Member Author

sourishkrout commented Oct 30, 2024

I see, @jlewi. We could employ the same strategy for non-interactive, which is already being used for interactive, where we create a single output with multiple items (different media types). This is how we render Gists with serialized output whereas the notebook shows a terminal.

For interactive, we have the following:

  • stateful.runme/terminal
  • application/vnd.code.notebook.stdout

and for non-interactive we could probably do:

  • stateful.runme/output-items
  • application/vnd.code.notebook.stdout
  • perhaps a third with application/vnd.code.notebook.stderr

Or combine stdout+stderr into a single application/vnd.code.notebook.stdout item.

@sourishkrout sourishkrout force-pushed the seb/better-non-tty-stdio branch 2 times, most recently from 9b988dc to a991d85 Compare November 22, 2024 17:17
@sourishkrout sourishkrout force-pushed the seb/better-non-tty-stdio branch from a991d85 to 3ce5cae Compare November 26, 2024 23:57
@sourishkrout sourishkrout marked this pull request as ready for review November 27, 2024 16:56
@sourishkrout
Copy link
Member Author

@pastuxso please review.

Copy link
Contributor

@pastuxso pastuxso left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@sourishkrout sourishkrout merged commit f83156a into main Dec 2, 2024
1 check passed
@sourishkrout sourishkrout deleted the seb/better-non-tty-stdio branch December 2, 2024 18:46
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

Successfully merging this pull request may close these issues.

3 participants