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

add mode for python execution #3713

Closed

Conversation

Igniscode
Copy link

divide docker mode and venv mode for python execution

Background

To run execute_python_file, which is currently built into autogpt, user needs to install docker and create docker-image separately, which you feel is unnecessary and unreasonable. (I watched multiple issues because of this, for example #92 #1896 )
It is likely to be a difficult task for beginners or those who are not familiar with Docker use.

Changes

Therefore, in python execution, the venv mode and the Docker mode were separated and implemented so that the user could choose how to execute the code in the .env file.

Documentation

Relevant information is annotated in code as comments

Test Plan

Same environment, I tested docker mode(legacy) and venv mode and they work same.

PR Quality Checklist

  • [v ] My pull request is atomic and focuses on a single change.
  • [v ] I have thoroughly tested my changes with multiple different prompts.
  • [v ] I have considered potential risks and mitigations for my changes.
  • [v ] I have documented my changes clearly and comprehensively.
  • [v ] I have not snuck in any "extra" small tweaks changes

divide docker mode and venv mode for python execution
@vercel
Copy link

vercel bot commented May 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview May 18, 2023 4:57am

@github-actions github-actions bot added the size/l label May 2, 2023
@p-i-
Copy link
Contributor

p-i- commented May 5, 2023

This is a mass message from the AutoGPT core team.
Our apologies for the ongoing delay in processing PRs.
This is because we are re-architecting the AutoGPT core!

For more details (and for infor on joining our Discord), please refer to:
https://github.com/Significant-Gravitas/Auto-GPT/wiki/Architecting

@Boostrix
Copy link
Contributor

Boostrix commented May 7, 2023

if this is committed "as is", it would make sense for auto-gpt to run inside a separate environment - i.e. using chroot or ideally using a dedicated "autogpt" user. That way, there's another layer of security added when it comes to $HOME

The same restriction should then apply for execute_shell commands obviously.
if $USER is not "autogpt", the script should internally disable code execution completely.

@Igniscode
Copy link
Author

if this is committed "as is", it would make sense for auto-gpt to run inside a separate environment - i.e. using chroot or ideally using a dedicated "autogpt" user. That way, there's another layer of security added when it comes to $HOME

The same restriction should then apply for execute_shell commands obviously. if $USER is not "autogpt", the script should internally disable code execution completely.

Thank you for the excellent feedback on my request. While using venv provides some minimum security, adding a new account seems like a much safer idea.

@Boostrix
Copy link
Contributor

Boostrix commented May 8, 2023

I did copy that advice into the source tree and opened a corresponding PR:

@Igniscode
Copy link
Author

I did copy that advice into the source tree and opened a corresponding PR:

Cool! I will also consider implementing rules for executing code based on user permissions.

@Boostrix
Copy link
Contributor

Boostrix commented May 8, 2023

there are already some hard-coded heuristics to detect if agpt is running inside docker - I suppose the "separate user" idea could be implemented via a corresponding env file setting, and that would simply require autogpt to run as the user "autogpt", and if it doesn't, disable executive functions - if anybody disagrees, they can edit the env file and opt out obviously

@Igniscode
Copy link
Author

there are already some hard-coded heuristics to detect if agpt is running inside docker - I suppose the "separate user" idea could be implemented via a corresponding env file setting, and that would simply require autogpt to run as the user "autogpt", and if it doesn't, disable executive functions - if anybody disagrees, they can edit the env file and opt out obviously

I understand your point, I think the "separate user" idea should be implemented in a separate pull request because it may have implications for other commands. Thanks for inform it!

@Boostrix
Copy link
Contributor

Boostrix commented May 8, 2023

sure, they won't accept multiple features in a single PR anyway

Also see:

@vercel vercel bot temporarily deployed to Preview May 15, 2023 08:06 Inactive
@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Patch coverage: 43.63% and project coverage change: -0.41 ⚠️

Comparison is base (dc41db9) 62.67% compared to head (8e424e8) 62.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3713      +/-   ##
==========================================
- Coverage   62.67%   62.27%   -0.41%     
==========================================
  Files          74       74              
  Lines        3400     3430      +30     
  Branches      495      501       +6     
==========================================
+ Hits         2131     2136       +5     
- Misses       1120     1143      +23     
- Partials      149      151       +2     
Impacted Files Coverage Δ
autogpt/main.py 0.00% <0.00%> (ø)
autogpt/commands/execute_code.py 53.19% <42.00%> (-16.38%) ⬇️
autogpt/config/config.py 75.00% <75.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vercel vercel bot temporarily deployed to Preview May 17, 2023 22:43 Inactive
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label May 22, 2023
@github-actions
Copy link
Contributor

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

@Pwuts
Copy link
Member

Pwuts commented Jun 7, 2023

Does venv provide the same degree of sandboxing as docker?

@Boostrix
Copy link
Contributor

Boostrix commented Jun 7, 2023

No.

@Pwuts
Copy link
Member

Pwuts commented Jun 7, 2023

Since venv does not provide the same degree of sandboxing as docker, they are only interchangeable if the user knows what they are doing and have purposefully disabled the workspace restriction.

@Pwuts
Copy link
Member

Pwuts commented Jun 7, 2023

Instead of merging this, I'd like to assure you that we are working on an improvement to the CLI that should make it a lot easier to run Auto-GPT (with docker).

@Pwuts Pwuts closed this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Automatically applied to PRs with merge conflicts size/l
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants