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 workspace abstraction #2982

Merged

Conversation

collijk
Copy link
Contributor

@collijk collijk commented Apr 23, 2023

Background

This is the first change on the path to building a self-contained "AICore" in line with our project plan, and focuses on wrangling the Agent workspace into a coherent abstraction. It gathers workspace path resolution into a class that the Agent instantiates and uses.

Due to the prior global nature of the existingworkspace, this change necessarily touches a large number of files, but should radically reduce the scope of certain kinds of future PRs. Importantly, despite the breadth of this PR, this code change is structured to require no behavior on the user side, however (except, of course, for those users hacking deeply into the codebase).

Changes

All changes here are in service of creating a workspace abstraction and removing the workspace from the global namespace.

  • Add a workspace_directory argument to the Agent used to construct the Agent's workspace abstraction.
  • Collect workspace path resolution into an Agent helper function.
  • Add a (for now) hidden argument to the app cli to allow the specification of a workspace directory
  • Collected workspace and logger file generation into the main function of the cli with appropriate config modification.
  • Commands are no longer responsible for path resolution, so all usages of path_in_workspace have been removed.
  • WORKSPACE_PATH no longer exists, so commands (and other code) now access it from the config (for now)
  • Added workspace_path and file_logger_path to the config.
  • Added a workspace package and module and a Workspace class to do the path resolution work.
  • Made the conditional skip of the image gen tests unconditional. They were broken and tests that don't run on CI are not useful tests.
  • Added test suite for workspace
  • Updated remaining test suite to account for the reduction in global state.

Could be separate, but I uncovered these issues in testing

  • Added functools.wraps decorators to the command decorator and the `to allow pytest fixtures to work correctly.

Documentation

All public functions have complete typing and docstrings and any complex logic is accompanied by comments.

Test Plan

~40 tests submitted to accompany code changes and all current unit and integration tests pass.

PR Quality Checklist

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

@@ -226,3 +232,14 @@ def start_interaction_loop(self):
logger.typewriter_log(
"SYSTEM: ", Fore.YELLOW, "Unable to execute command"
)

def _resolve_pathlike_command_args(self, command_args):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll want some kind of introspection mechanism for pathlike command args at some point.

@@ -164,6 +174,27 @@ def main(
system_prompt = ai_config.construct_full_prompt()
if cfg.debug_mode:
logger.typewriter_log("Prompt:", Fore.GREEN, system_prompt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main purpose of the addition here is to pull the creation of the workspace and file logger file from import time and remove them from the global namespace.

@@ -43,12 +40,7 @@ def log_operation(operation: str, filename: str) -> None:
filename (str): The name of the file the operation was performed on
"""
log_entry = f"{operation}: {filename}\n"

# Create the log file if it doesn't exist
if not os.path.exists(LOG_FILE_PATH):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done at app start

@@ -222,16 +210,13 @@ def search_files(directory: str) -> list[str]:
"""
found_files = []

if directory in {"", "/"}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now done in agent path resolution

@@ -28,19 +25,12 @@


@requires_api_key("OPENAI_API_KEY")
def test_write_file() -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The introduction of a pytest fixture that does its own cleanup removes a ton of boilerplate setup/teardown from tests

tests/smoke_test.py Outdated Show resolved Hide resolved
@@ -24,24 +23,24 @@ class TestFileOperations(unittest.TestCase):
"""

def setUp(self):
self.test_file = "test_file.txt"
self.config = Config()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone knows how to make unittest classes use pytest fixtures, let me know. Otherwise I think we should add a backlog item to refactor things so we're using pytest only.

@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Patch coverage: 55.29% and project coverage change: -8.38 ⚠️

Comparison is base (da48f9c) 45.81% compared to head (5908137) 37.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2982      +/-   ##
==========================================
- Coverage   45.81%   37.44%   -8.38%     
==========================================
  Files          60       61       +1     
  Lines        2918     2938      +20     
  Branches      480      490      +10     
==========================================
- Hits         1337     1100     -237     
- Misses       1472     1778     +306     
+ Partials      109       60      -49     
Impacted Files Coverage Δ
autogpt/cli.py 0.00% <0.00%> (ø)
autogpt/commands/audio_text.py 0.00% <0.00%> (ø)
autogpt/commands/execute_code.py 0.00% <0.00%> (ø)
autogpt/commands/git_operations.py 0.00% <0.00%> (ø)
autogpt/commands/image_gen.py 26.41% <0.00%> (-1.37%) ⬇️
autogpt/agent/agent.py 13.27% <18.18%> (-27.91%) ⬇️
autogpt/commands/file_operations.py 56.80% <90.00%> (-4.36%) ⬇️
autogpt/commands/command.py 77.94% <100.00%> (+0.66%) ⬆️
autogpt/config/config.py 74.83% <100.00%> (-1.01%) ⬇️
autogpt/workspace/__init__.py 100.00% <100.00%> (ø)
... and 1 more

... and 14 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

tests/conftest.py Show resolved Hide resolved
tests/test_image_gen.py Show resolved Hide resolved
tests/test_image_gen.py Show resolved Hide resolved
tests/test_workspace.py Show resolved Hide resolved
tests/unit/test_file_operations.py Show resolved Hide resolved
@collijk collijk requested a review from ntindle April 23, 2023 07:51
Copy link
Member

@ntindle ntindle left a comment

Choose a reason for hiding this comment

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

Need to run locally before merge. Don’t have laptop handy but code LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants