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:
|
![codiumai-pr-agent-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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.
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.
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.
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.
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.
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.
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. |
Pwuts
changed the title
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
github-actions
bot
added
the
conflicts
…broken-on-non-local-file-storage
github-actions
bot
removed
the
conflicts
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
…broken-on-non-local-file-storage
github-actions
bot
added
the
conflicts
|
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. |
github-actions
bot
added
conflicts
github-actions
bot
removed
the
conflicts
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
Pwuts
changed the title
mount method to FileStorage & execute code in mounted workspace
kcze
deleted the
kpczerwinski/open-753-execute_python_file-broken-on-non-local-file-storage
branch
User description
Background
Python file execution doesn't work when non-local
FileStorageis used.Changes 🏗️
In
forge:FileStorage.mount()method, which mounts (part of) the workspace to a local pathwatchdoglibrary to watch file changes in mountCodeExecutorComponentexecute_python_fileto execute Python files in a workspace mountexecute_python_codeto create temporary .py file in workspace instead of as a local filePathargument tofilenameparameter onexecute_python_fileIn
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 ptsagbenchmarkto verify that these changes do not regress performance?+10 ptsType
enhancement
Description
execute_python_codeasynchronous and improving file handling.FileSyncHandlerand context manager for mounting directories.watchdogfor monitoring file system events.Changes walkthrough
execute_code.py
Enhance Python code execution and file handlingautogpts/autogpt/autogpt/commands/execute_code.py
execute_python_codeto an asynchronous function.workspace.
execute_python_fileto support execution in a Dockercontainer with mounted local paths.
_generate_random_stringto create randomfilenames.
base.py
Implement file synchronization and temporary mountingautogpts/autogpt/autogpt/file_storage/base.py
FileSyncHandlerclass to synchronize file operations with thestorage.
mountto handle temporary localdirectories.
local.py
Add local storage mounting capabilityautogpts/autogpt/autogpt/file_storage/local.py
mountmethod as a context manager for local file storage.pyproject.toml
Update project dependenciesautogpts/autogpt/pyproject.toml
watchdogdependency to monitor file system events.