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

feat(agent): Persistent execute_code session #7078

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MKdir98
Copy link
Contributor

@MKdir98 MKdir98 commented Apr 8, 2024

  • Add notebook libs
  • Add two tests for the execute_code part
  • Add a Python kernel to each agent
  • Change the execute_code command part to persist the state of the codes

Background

As we talked about in Persistent Python session / Jupyter Notebook integration , we need to change the execute_code command to save the state of each executed code. We attach a Python kernel and a notebook to each agent in initializing the agent part. After that now we can use that kernel to run Python code with that kernel and this kernel will save the session until the agent gets destroyed. The error part may be a little dirty and I think maybe we can find something better instead of kernel.execute. You can check tests for execute_code to see scenarios we can support right now.

Changes πŸ—οΈ

In the execute_code command instead of creating a temp file and running that file, we run the code directly by python kernel.

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

Copy link

github-actions bot commented Apr 8, 2024

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Copy link

netlify bot commented Apr 8, 2024

βœ… Deploy Preview for auto-gpt-docs canceled.

Name Link
πŸ”¨ Latest commit 02897f4
πŸ” Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6638e4b505296d00086db219

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2024
Copy link

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@Swiftyos
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Apr 23, 2024

PR Review

(Review updated until commit 60cdfd7)

⏱️ Estimated effort to review [1-5]

4, due to the complexity of integrating Jupyter kernels and the need to ensure that the implementation does not introduce any security vulnerabilities or logical errors in handling kernel sessions and outputs.

πŸ§ͺΒ Relevant tests

Yes

πŸ”Β Possible issues

Possible Bug: The error handling in execute_python_code might not correctly format exceptions. The CommandExecutionError is raised with *e.args which might not correctly pass the exception details if e has more than one argument.

Resource Management: The kernel sessions started with start_new_kernel are not explicitly terminated or cleaned up, which could lead to resource leaks if kernels are not properly managed.

πŸ”’Β Security concerns

No

Code feedback:
relevant fileautogpts/autogpt/autogpt/agents/base.py
suggestion Β Β Β Β Β 

Consider managing the lifecycle of the Python kernel to prevent resource leaks. You should ensure that kernels are properly shut down when they are no longer needed or when the agent is destroyed. This can be done by implementing a cleanup method in the BaseAgentSettings class that calls self.python_kernel.shutdown(). [important]

relevant lineself.python_kernel = start_new_kernel(

relevant fileautogpts/autogpt/autogpt/commands/execute_code.py
suggestion Β Β Β Β Β 

Improve the exception handling in execute_python_code to ensure that all parts of the exception are correctly formatted and passed to CommandExecutionError. Instead of using *e.args, you might want to directly pass the string representation of the exception or format it to include more context. [important]

relevant lineraise CommandExecutionError(*e.args)

relevant fileautogpts/autogpt/autogpt/commands/execute_code.py
suggestion Β Β Β Β Β 

To enhance code readability and maintainability, consider abstracting the logic that handles the output and errors from the kernel into separate methods. This can help in isolating the functionality and making the code easier to manage and test. [medium]

relevant lineagent.python_kernel.execute(code)

relevant fileautogpts/autogpt/autogpt/agents/base.py
suggestion Β Β Β Β Β 

Add error handling for the kernel start-up process in the __init__ method of BaseAgentSettings. This should manage scenarios where the kernel fails to start, possibly due to configuration issues or system limitations. [important]

relevant lineself.python_kernel = start_new_kernel(


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Conflicts have been resolved! πŸŽ‰ A maintainer will review the pull request shortly.

@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch from 970c95c to 60cdfd7 Compare May 2, 2024 12:45
@MKdir98
Copy link
Contributor Author

MKdir98 commented May 2, 2024

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 60cdfd7

@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch 2 times, most recently from 42b8c17 to 4f91372 Compare May 2, 2024 14:16
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 44.81%. Comparing base (9bac6f4) to head (02897f4).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7078      +/-   ##
==========================================
+ Coverage   44.65%   44.81%   +0.16%     
==========================================
  Files         133      133              
  Lines        6306     6321      +15     
  Branches      822      824       +2     
==========================================
+ Hits         2816     2833      +17     
+ Misses       3379     3377       -2     
  Partials      111      111              
Flag Coverage Ξ”
Linux 44.73% <100.00%> (+0.16%) ⬆️
Windows 42.72% <34.78%> (-0.01%) ⬇️
autogpt-agent 44.78% <100.00%> (+0.16%) ⬆️
macOS 43.94% <34.78%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch 5 times, most recently from 23ffd50 to 97ac81b Compare May 2, 2024 14:56
Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this! It needs a bit of work but it will be very cool to get this merged :)

autogpts/autogpt/autogpt/agents/base.py Outdated Show resolved Hide resolved
autogpts/autogpt/autogpt/agents/components.py Outdated Show resolved Hide resolved
autogpts/autogpt/autogpt/agents/base.py Outdated Show resolved Hide resolved
autogpts/autogpt/autogpt/commands/execute_code.py Outdated Show resolved Hide resolved
autogpts/autogpt/pyproject.toml Outdated Show resolved Hide resolved
autogpts/autogpt/tests/integration/test_execute_code.py Outdated Show resolved Hide resolved
autogpts/autogpt/tests/integration/test_execute_code.py Outdated Show resolved Hide resolved
@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch 3 times, most recently from 28b9510 to 6a8e728 Compare May 3, 2024 13:59
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label May 4, 2024
Copy link

github-actions bot commented May 4, 2024

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch from 6a8e728 to 4fabc34 Compare May 6, 2024 14:06
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label May 6, 2024
Copy link

github-actions bot commented May 6, 2024

Conflicts have been resolved! πŸŽ‰ A maintainer will review the pull request shortly.

- Add notebook libs
- Add two tests for the execute_code part
- Add a Python kernel to each agent
- Change the execute_code command part to persist the state of the codes
@MKdir98 MKdir98 force-pushed the feat/jupyter-notebook-integration branch from 4fabc34 to 02897f4 Compare May 6, 2024 14:09
Copy link
Contributor

@kcze kcze left a comment

Choose a reason for hiding this comment

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

This looks ok to me overall and;

Comment on lines -336 to +347
benchmark = ["agbenchmark @ git+https://github.com/Significant-Gravitas/AutoGPT.git#subdirectory=benchmark"]
benchmark = ["agbenchmark @ file:///home/mk/.cache/pypoetry/virtualenvs/agpt-xgev2_OR-py3.11/src/AutoGPT/benchmark"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This happens to me for some reason as well, please revert this to git address and be careful when updating poetry packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sure thanks

@MKdir98
Copy link
Contributor Author

MKdir98 commented May 8, 2024

This looks ok to me overall and;

OK. I'll test it with agent protocol

if not self.legacy_config.execute_local_commands:
logger.info(
"Local shell commands are disabled. To enable them,"
" set EXECUTE_LOCAL_COMMANDS to 'True' in your config file."
)
self.notebook = new_notebook()
Copy link
Member

Choose a reason for hiding this comment

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

With this, does it always make a new notebook on init between instances of the agent? Will that session be shared across multiple agents running within the agent protocol service?

@kcze not sure how components are init'ed yet

def _execute_code_in_agent_python_session(self, code: str) -> str:
"""
This function will run code on python_kernel.`self.python_kernel.execute(code)`
does not return stdout and error direclty.The while part
Copy link
Member

@ntindle ntindle May 9, 2024

Choose a reason for hiding this comment

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

nit

Suggested change
does not return stdout and error direclty.The while part
does not return stdout and error directly. The while part

Comment on lines +148 to +151
raise IOError(
f"{io_msg['content']['evalue']}\n \
{io_msg['content']['traceback']}"
)
Copy link
Member

Choose a reason for hiding this comment

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

nit: is IOError appropriate here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🚧 Needs work
Development

Successfully merging this pull request may close these issues.

None yet

6 participants