-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Process interactive commands and stream output in logs #2059
Changes from 9 commits
d8d17aa
1947629
9813637
3477e00
49d464a
99dfea5
e21933c
233fcd1
b2d5939
1756abc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
from glob import glob | ||
|
||
import docker | ||
from pexpect import exceptions, pxssh | ||
from pexpect import EOF, TIMEOUT, ExceptionPexpect, exceptions, pxssh | ||
from tenacity import retry, stop_after_attempt, wait_fixed | ||
|
||
from opendevin.core.config import config | ||
|
@@ -45,7 +45,11 @@ def exit_code(self): | |
return -1 | ||
|
||
_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 | ||
|
||
def read_output(self): | ||
st = time.time() | ||
|
@@ -455,10 +459,38 @@ def execute( | |
self.ssh.sendline(cmd) | ||
if stream: | ||
return 0, SSHExecCancellableStream(self.ssh, cmd, self.timeout) | ||
success = self.ssh.prompt(timeout=timeout) | ||
if not success: | ||
return self._send_interrupt(cmd) | ||
command_output = self.ssh.before | ||
prompts = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about we inherit from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will we move to Paramiko for windows support? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that would be better for connecting the frontend terminal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In which file, do you want this to be implemented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still need to inherit as it is reaching the end of life? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so! We can leave it as is for now probably? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you reopen it? |
||
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 | ||
Comment on lines
+473
to
+504
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is introducing several changes in the way that prompts are handled.
|
||
|
||
# once out, make sure that we have *every* output, we while loop until we get an empty output | ||
while True: | ||
|
@@ -475,7 +507,7 @@ def execute( | |
) | ||
if isinstance(output, str) and output.strip() == '': | ||
break | ||
command_output += output | ||
command_output += str(output) | ||
command_output = command_output.removesuffix('\r\n') | ||
|
||
# get the exit code | ||
|
@@ -735,11 +767,9 @@ def close(self): | |
except Exception as e: | ||
logger.exception('Failed to start Docker container: %s', e) | ||
sys.exit(1) | ||
|
||
logger.info( | ||
"Interactive Docker container started. Type 'exit' or use Ctrl+C to exit." | ||
) | ||
|
||
# Initialize required plugins | ||
ssh_box.init_plugins([AgentSkillsRequirement(), JupyterRequirement()]) | ||
logger.info( | ||
|
@@ -749,7 +779,7 @@ def close(self): | |
) | ||
|
||
bg_cmd = ssh_box.execute_in_background( | ||
"while true; do echo -n '.' && sleep 10; done" | ||
"while true; do echo -n '.' && sleep 20; done" | ||
) | ||
|
||
sys.stdout.flush() | ||
|
@@ -769,7 +799,6 @@ def close(self): | |
continue | ||
exit_code, output = ssh_box.execute(user_input) | ||
logger.info('exit code: %d', exit_code) | ||
logger.info(output) | ||
if bg_cmd.pid in ssh_box.background_commands: | ||
logs = ssh_box.read_logs(bg_cmd.pid) | ||
logger.info('background logs: %s', logs) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to check the error rather than use
try catch
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-L58There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC: @neubig, I am waiting for the reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with @yufansong that we should fix the underlying problem, rather than catching the error here.