-
Notifications
You must be signed in to change notification settings - Fork 43k
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(forge): Add mount
method to FileStorage
& execute code in mounted workspace
#7115
feat(forge): Add mount
method to FileStorage
& execute code in mounted workspace
#7115
Conversation
Allow to use synchronized local temp dir
✅ Deploy Preview for auto-gpt-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7115 +/- ##
=======================================
Coverage 36.05% 36.05%
=======================================
Files 19 19
Lines 1273 1273
Branches 182 182
=======================================
Hits 459 459
Misses 786 786
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Make temp file manually
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Description updated to latest commit (86c5dc7)
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Review
Code feedback:
|
temp_path = Path("") | ||
while True: | ||
temp_path = f"temp{self._generate_random_string()}.py" | ||
if not self.workspace.exists(temp_path): | ||
break | ||
await self.workspace.write_file(temp_path, code) |
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.
Suggestion: Replace the manual loop for checking if a file exists with a more robust temporary file creation using the tempfile
module. This avoids potential race conditions and simplifies the code. [enhancement]
temp_path = Path("") | |
while True: | |
temp_path = f"temp{self._generate_random_string()}.py" | |
if not self.workspace.exists(temp_path): | |
break | |
await self.workspace.write_file(temp_path, code) | |
with NamedTemporaryFile(suffix=".py", dir=self.workspace.root, delete=False) as tmp_file: | |
temp_path = tmp_file.name | |
await self.workspace.write_file(temp_path, code) |
file_path = self.workspace.get_path(filename) | ||
if not self.workspace.exists(file_path): | ||
# Mimic the response that you get from the command line to make it | ||
# intuitively understandable for the LLM | ||
raise FileNotFoundError( |
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.
Suggestion: Use a context manager to handle file operations to ensure that files are always closed properly, even if an error occurs. [best practice]
file_path = self.workspace.get_path(filename) | |
if not self.workspace.exists(file_path): | |
# Mimic the response that you get from the command line to make it | |
# intuitively understandable for the LLM | |
raise FileNotFoundError( | |
with self.workspace.get_path(filename) as file_path: | |
if not self.workspace.exists(file_path): | |
raise FileNotFoundError( | |
"AutoGPT is running in a Docker container; " | |
f"executing {file_path} directly..." | |
) |
if isinstance(args, str): | ||
args = args.split() # Convert space-separated string to a list |
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.
Suggestion: Instead of manually checking if args
is a string and then splitting it, use a more Pythonic approach by ensuring args
is always a list at the start of the function. [best practice]
if isinstance(args, str): | |
args = args.split() # Convert space-separated string to a list | |
args = shlex.split(args) if isinstance(args, str) else args |
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.
Suggested change does not match suggestion description. 👎🏼
BUT we do argument validation so this block can be removed entirely.
with open(file_path, "rb") as f: | ||
content = f.read() | ||
assert isinstance(content, bytes) | ||
await self.storage.write_file(file_path, content) |
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.
Suggestion: Use a more efficient method for reading file content into memory by leveraging read()
directly on the file object opened in binary mode, which is more memory efficient for large files. [performance]
with open(file_path, "rb") as f: | |
content = f.read() | |
assert isinstance(content, bytes) | |
await self.storage.write_file(file_path, content) | |
with open(file_path, "rb") as f: | |
await self.storage.write_file(file_path, f.read()) |
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 think the proposed change has zero effect on memory consumption, as the argument f.read()
will still be evaluated and executed before write_file
is called.
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. Changelog updates: 2024-04-30Added
Changed
Fixed
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Analysis
✨ Usage guide:Using static code analysis capabilities, the
Language that are currently supported: Python, Java, C++, JavaScript, TypeScript. |
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.
A few comments, but overall I like the setup!
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
…broken-on-non-local-file-storage
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
…broken-on-non-local-file-storage
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
…broken-on-non-local-file-storage
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
mount
method to FileStorage
& execute code in mounted workspace
User description
Background
Python file execution doesn't work when non-local
FileStorage
is used.Changes 🏗️
In
forge
:FileStorage.mount()
method, which mounts (part of) the workspace to a local pathwatchdog
library to watch file changes in mountCodeExecutorComponent
execute_python_file
to execute Python files in a workspace mountexecute_python_code
to create temporary .py file in workspace instead of as a local filePath
argument tofilename
parameter onexecute_python_file
In
autogpt
:test_execute_python_code
(by making it async)PR Quality Scorecard ✨
+2 pts
+5 pts
+5 pts
+5 pts
-4 pts
+4 pts
+5 pts
-5 pts
agbenchmark
to verify that these changes do not regress performance?+10 pts
Type
enhancement
Description
execute_python_code
asynchronous and improving file handling.FileSyncHandler
and context manager for mounting directories.watchdog
for monitoring file system events.Changes walkthrough
execute_code.py
Enhance Python code execution and file handling
autogpts/autogpt/autogpt/commands/execute_code.py
execute_python_code
to an asynchronous function.workspace.
execute_python_file
to support execution in a Dockercontainer with mounted local paths.
_generate_random_string
to create randomfilenames.
base.py
Implement file synchronization and temporary mounting
autogpts/autogpt/autogpt/file_storage/base.py
FileSyncHandler
class to synchronize file operations with thestorage.
mount
to handle temporary localdirectories.
local.py
Add local storage mounting capability
autogpts/autogpt/autogpt/file_storage/local.py
mount
method as a context manager for local file storage.pyproject.toml
Update project dependencies
autogpts/autogpt/pyproject.toml
watchdog
dependency to monitor file system events.