Process interactive commands and stream output in logs #2059

SmartManoj · 2024-05-25T15:36:52Z

Solves #577 and also streaming output to logs only #1625 (comment)

yufansong · 2024-05-26T03:42:59Z

opendevin/runtime/docker/ssh_box.py

@@ -738,21 +769,21 @@ def close(self):
    except Exception as e:
        logger.exception('Failed to start Docker container: %s', e)
        sys.exit(1)
-
+    logger.setLevel('DEBUG')


Forget to delete or intentional?

intentional; as it is for devs

I am strongly against changing logger level in the code

It is in the main block only.

It is in the main block only.

Ah I see, that's much less concerning then... I still prefer we don't do it in the code, coz it might surprise developers

Maybe logging about the change would help?

opendevin/runtime/docker/ssh_box.py

yufansong · 2024-05-26T03:43:53Z

opendevin/runtime/docker/ssh_box.py

        _exit_code = self.ssh.before.strip()
-        return int(_exit_code)
+        try:
+            return int(_exit_code)
+        except ValueError:
+            logger.error(f'Invalid exit code: {_exit_code}')
+            return -1


I prefer to check the error rather than use try catch.

Got Jupyter kernel ready in _exit_code and the program stopped when I set timeout to 3 for testing. There some bugs in that like #1915. Also, it is handled in https://github.com/OpenDevin/OpenDevin/blob/d8d17aae03c8c0987cbce625103cd15020fb05da/opendevin/runtime/plugins/mixin.py#L55-L58

CC: @neubig, I am waiting for the reply.

Yeah, I agree with @yufansong that we should fix the underlying problem, rather than catching the error here.

li-boxuan · 2024-05-26T03:55:54Z

opendevin/runtime/docker/ssh_box.py

@@ -738,21 +769,21 @@ def close(self):
    except Exception as e:
        logger.exception('Failed to start Docker container: %s', e)
        sys.exit(1)
-
+    logger.setLevel('DEBUG')


I am strongly against changing logger level in the code

yufansong

I think some code need to be refactor.

neubig · 2024-05-31T19:15:00Z

@SmartManoj : if this is ready for review again, could you re-request review? You can do it by clicking the "cycle" button next to the reviewers' names.

reverted to old stated.

li-boxuan · 2024-06-01T07:07:23Z

I don't understand what problem this PR is trying to solve, and how it is solved. Do you mind elaborating more?

SmartManoj · 2024-06-01T07:13:52Z

Solves #577

It will parse the output and if it requires "yes" confirmation from the user, it will send that.

rbren · 2024-06-03T13:59:36Z

opendevin/runtime/docker/ssh_box.py

+        prompts = [
+            r'Do you want to continue\? \[Y/n\]',
+            self.ssh.PROMPT,
+            EOF,
+            TIMEOUT,
+        ]
+        command_output = ''
+        timeout_counter = 0
+        while True:
+            try:
+                # Wait for one of the prompts
+                index = self.ssh.expect(prompts, timeout=1)
+                line = self.ssh.before
+                logger.info(line)
+                command_output += line
+                if index == 0:
+                    self.ssh.sendline('Y')
+                elif index == 1:
+                    break
+                elif index == 2:
+                    logger.debug('End of file')
+                    break
+                elif index == 3:
+                    timeout_counter += 1
+                    if timeout_counter > timeout:
+                        logger.exception(
+                            'Command timed out, killing process...', exc_info=False
+                        )
+                        return self._send_interrupt(cmd)
+            except ExceptionPexpect as e:
+                logger.exception(f'Unexpected exception: {e}')
+                break


This seems brittle to me. Auto-confirm based on prompt text is kind of a hack

It looks like you have another PR which warns the agent about commands like this...will that suffice?

I'm generally concerned that this file is filling up with little hacks for the agent, instead of just being a thin wrapper around SSH.

Did you mean #2034 (I'm not the author)? We can warn them but they are not AGI models yet to follow that always. So need some little tweaks. Also helps weaker LLMs

This is introducing several changes in the way that prompts are handled.

Automatic entering of "Y" -- I think this is a bad idea... Usually a confirmation dialog is there because you need to check before doing something dangerous, and having the system automatically choose to proceed seems dangerous.

This does nothing, so it can probably just be removed?

Adding "end of file" may be good, but what was the behavior before?

This seems like a good idea, and I actually like it better than the solution in fix: codeact bug [If running a command that never returns, it gets stuck #1895] #2034, which expands the prompt for something that can probably be done automatically like we do it here.

neubig

Thanks for this! I had a few comments.

neubig · 2024-06-08T11:25:49Z

opendevin/runtime/docker/ssh_box.py

+        prompts = [
+            r'Do you want to continue\? \[Y/n\]',
+            self.ssh.PROMPT,
+            EOF,
+            TIMEOUT,
+        ]
+        command_output = ''
+        timeout_counter = 0
+        while True:
+            try:
+                # Wait for one of the prompts
+                index = self.ssh.expect(prompts, timeout=1)
+                line = self.ssh.before
+                logger.info(line)
+                command_output += line
+                if index == 0:
+                    self.ssh.sendline('Y')
+                elif index == 1:
+                    break
+                elif index == 2:
+                    logger.debug('End of file')
+                    break
+                elif index == 3:
+                    timeout_counter += 1
+                    if timeout_counter > timeout:
+                        logger.exception(
+                            'Command timed out, killing process...', exc_info=False
+                        )
+                        return self._send_interrupt(cmd)
+            except ExceptionPexpect as e:
+                logger.exception(f'Unexpected exception: {e}')
+                break


This is introducing several changes in the way that prompts are handled.

Automatic entering of "Y" -- I think this is a bad idea... Usually a confirmation dialog is there because you need to check before doing something dangerous, and having the system automatically choose to proceed seems dangerous.

This does nothing, so it can probably just be removed?

Adding "end of file" may be good, but what was the behavior before?

This seems like a good idea, and I actually like it better than the solution in fix: codeact bug [If running a command that never returns, it gets stuck #1895] #2034, which expands the prompt for something that can probably be done automatically like we do it here.

neubig · 2024-06-08T11:27:31Z

opendevin/runtime/docker/ssh_box.py

        _exit_code = self.ssh.before.strip()
-        return int(_exit_code)
+        try:
+            return int(_exit_code)
+        except ValueError:
+            logger.error(f'Invalid exit code: {_exit_code}')
+            return -1


Yeah, I agree with @yufansong that we should fix the underlying problem, rather than catching the error here.

SmartManoj · 2024-06-08T11:30:17Z

Automatic entering of "Y" will only affect the sandbox.
We are exiting the loop. ( you mean index==1?)
End-of-file error was not observed before.

xingyaoww · 2024-05-26T14:16:13Z

opendevin/runtime/docker/ssh_box.py

-            logger.exception('Command timed out, killing process...', exc_info=False)
-            return self._send_interrupt(cmd)
-        command_output = self.ssh.before
+        prompts = [


How about we inherit from pxssh.pxssh, and override their .prompt method to implement this? It can make the code much more readable?

Will we move to Paramiko for windows support?

In the next week, I'm thinking about completely get rid of SSH, and spin up a websocket client inside sandbox, connect it to the backend, and do things from there -- this will allow us to use ONE event stream for everything which can be cleaner. I can start a draft PR and we can discuss from there i guess.

Yeah, that would be better for connecting the frontend terminal.

How about we inherit from pxssh.pxssh, and override their .prompt method to implement this? It can make the code much more readable?

In which file, do you want this to be implemented?

still need to inherit as it is reaching the end of life?

I don't think so! We can leave it as is for now probably?

Would you reopen it?

mamoodi · 2024-06-30T15:24:47Z

@SmartManoj wondering if this is going to be continued or should be closed?

xingyaoww · 2024-07-04T15:12:48Z

Let's close it for now since it has been stale for a while! Feel free to reopen if things progress and need review.

SmartManoj · 2024-07-04T15:53:19Z

Discussion with @mamoodi on slack,

#2404

SmartManoj · 2024-07-20T02:51:33Z

@neubig

Automatic entering of "Y" will only affect the sandbox.

Confirmation mode will handle this now.

3. End-of-file error was not observed before.

#2427 (comment)

SmartManoj added 2 commits May 25, 2024 20:57

Allowed interactive commands

d8d17aa

Merge branch 'main' into sshbox

1947629

yufansong reviewed May 26, 2024

View reviewed changes

li-boxuan previously requested changes May 26, 2024

View reviewed changes

yufansong requested changes May 26, 2024

View reviewed changes

SmartManoj added 4 commits May 26, 2024 09:33

reverted to old state in main block

9813637

Merge branch 'main' into sshbox

3477e00

captured last line

49d464a

Merge remote-tracking branch 'upstream/main' into sshbox

99dfea5

SmartManoj force-pushed the sshbox branch from 85e57de to 99dfea5 Compare May 26, 2024 11:02

assertion mentioned this pull request May 27, 2024

fix: codeact bug [If running a command that never returns, it gets stuck #1895] #2034

Merged

Merge branch 'main' into sshbox

e21933c

neubig assigned SmartManoj May 31, 2024

Merge branch 'main' into sshbox

233fcd1

SmartManoj requested review from yufansong and li-boxuan May 31, 2024 23:09

Merge branch 'main' into sshbox

b2d5939

SmartManoj changed the title ~~Allowed interactive commands~~ Processed interactive commands Jun 1, 2024

rbren reviewed Jun 3, 2024

View reviewed changes

SmartManoj changed the title ~~Processed interactive commands~~ Processed interactive commands and stream able output in logs Jun 4, 2024

SmartManoj changed the title ~~Processed interactive commands and stream able output in logs~~ Processed interactive commands and stream-able output in logs Jun 5, 2024

SmartManoj changed the title ~~Processed interactive commands and stream-able output in logs~~ Processed interactive commands and streamable output in logs Jun 5, 2024

Merge branch 'main' into sshbox

1756abc

neubig reviewed Jun 8, 2024

View reviewed changes

xingyaoww reviewed Jun 9, 2024

View reviewed changes

SmartManoj marked this pull request as draft June 16, 2024 06:22

xingyaoww closed this Jul 4, 2024

SmartManoj mentioned this pull request Jul 11, 2024

Feat: Add fast boot #2808

Closed

SmartManoj mentioned this pull request Jul 20, 2024

[Runtime] Mega-issue to track all issues related to bash Interactive terminal #3031

Closed

10 tasks

SmartManoj changed the title ~~Processed interactive commands and streamable output in logs~~ Process interactive commands and stream output in logs Jul 20, 2024

SmartManoj mentioned this pull request Jul 20, 2024

Process interactive commands and stream output in logs #3042

Closed

Process interactive commands and stream output in logs #2059

Process interactive commands and stream output in logs #2059

Conversation

SmartManoj commented May 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufansong left a comment

Choose a reason for hiding this comment

neubig commented May 31, 2024

li-boxuan commented Jun 1, 2024

SmartManoj commented Jun 1, 2024

Choose a reason for hiding this comment

SmartManoj Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neubig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmartManoj commented Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

SmartManoj Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmartManoj Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmartManoj Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

mamoodi commented Jun 30, 2024

xingyaoww commented Jul 4, 2024

SmartManoj commented Jul 4, 2024 • edited Loading

SmartManoj commented Jul 20, 2024 • edited Loading

SmartManoj Jun 3, 2024 •

edited

Loading

SmartManoj commented Jun 8, 2024 •

edited

Loading

SmartManoj Jun 9, 2024 •

edited

Loading

SmartManoj Jul 20, 2024 •

edited

Loading

SmartManoj Jul 20, 2024 •

edited

Loading

SmartManoj commented Jul 4, 2024 •

edited

Loading

SmartManoj commented Jul 20, 2024 •

edited

Loading