Feat: add stream output to exec_run #1625
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1625 +/- ##
=======================================
Coverage ? 64.73%
=======================================
Files ? 100
Lines ? 4143
Branches ? 0
=======================================
Hits ? 2682
Misses ? 1461
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
This is very cool! I'm a little worried about changing the footprint of Do you think it'd be possible to add this to SSH box too? And do you think it'd be possible to use this to stream output to the FE? |
i think every box should have a stream output as much as possible, they can easily implement the
yes, i wanna apply the stream to the SSH box. but i was too busy the last few days, i think i can work at it by the end of the week.
i think so, but it may have a bit of trouble when the concurrent sends messages from the BE to FE. |
* Using command timeout to control the exec_box's timeout. * add bash -c to source command to compatible for sh. Signed-off-by: ifuryst <ifuryst@gmail.com>
iFurySt
force-pushed
the
feat-add-stream-output-for-exec_run
branch
from
73430bd to
d680218
Compare
Signed-off-by: ifuryst <ifuryst@gmail.com>
iFurySt
force-pushed
the
feat-add-stream-output-for-exec_run
branch
from
0687d13 to
ef08149
Compare
Signed-off-by: ifuryst <ifuryst@gmail.com>
iFurySt
force-pushed
the
feat-add-stream-output-for-exec_run
branch
from
ef08149 to
1f561b0
Compare
Signed-off-by: ifuryst <ifuryst@gmail.com>
SSH.Box.execute.with.stream.output.screen.recording.mp4ignores the error for BrowserEnv, it has nothing to do with this pr. #350 |
|
@rbren @xingyaoww feel free to review it whenever you have time. |
|
This is looking good to me! I'm mostly worried about the complicated logic for parsing the SSH stream--nothing we can leverage from pexpect or another SSH lib? |
i can't found related lib to use. paramiko with paramiko-expect are similar with expect. the parse logic just buffer the output and then return line by line, and remove the PROMPT([PEXPECT]) finally. |
| @@ -344,21 +412,25 @@ def _send_interrupt( | |||
| f'Command: "{cmd}" timed out. Sending SIGINT to the process: {command_output}', | |||
| ) | |||
|
|
|||
| def execute(self, cmd: str, timeout: int | None = None) -> tuple[int, str]: | |||
| def execute( | |||
| self, cmd: str, stream: bool = False, timeout: int | None = None | |||
There was a problem hiding this comment.
Why stream is disabled by default?
There was a problem hiding this comment.
I think streaming output makes sense for initialization or time-consuming operations to get feedback. Currently, we don't support streaming output to the frontend. And the agent prefers not to receive the streaming output.
There was a problem hiding this comment.
How about only streaming the output to the logs?
@iFurySt Paramiko with continuous stdout You were looking for this? |
Screen.Recording.2024-05-07.at.22.38.44.mov
i found the init is too long and haven't feedback, and IMO i think the init does not even need to control the timeout. i failed to init a few times yesterday because my network seems poor, it made me a bit confused ~
i added the stream output for feedback and kept the timeout but changed to use the
timeoutcommand.