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(forge): Implement a very very bad, but "functional" agent by default #7021

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Mar 17, 2024

Background

We’ve received some notes about Forge being difficult to use and a bit broken. This PR aims to solve some of the core issues with that.

Changes 🏗️

  • Fixes various issues
    • Windows Compatibility
    • FastAPI interface Issues
    • Hardcoded paths removed
    • Formatting
    • Ability -> Action Fixes
    • Prompt
    • Imports
    • Actions Files themselves
    • Notes for the codebase
    • Endpoint Error message fixes
  • Updates typing throughout
  • Swaps agent from one that always replies Washington DC to one that has a very bad agent loop

Do we want to keep Washington DCcand make a real agent instead?

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

@ntindle ntindle requested a review from Pwuts March 17, 2024 05:36
@github-actions github-actions bot removed the Arena Agents Entering the Arena label Mar 17, 2024
Copy link

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 Mar 17, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 26b582c
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/663caca5d05ea400076e1685
😎 Deploy Preview https://deploy-preview-7021--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

description="The creation datetime of the task.",
example="2023-01-01T00:00:00Z",
json_encoders={datetime: lambda v: v.isoformat()},
Copy link
Member Author

Choose a reason for hiding this comment

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

This is why the openapi docs doesn't work btw

@@ -108,6 +108,7 @@ def load_prompt(self, template: str, **kwargs) -> str:
template = os.path.join(self.model, template)
if self.debug_enabled:
LOG.debug(f"Loading template: {template}")
template = template.replace("\\", "/")
Copy link
Member Author

Choose a reason for hiding this comment

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

windows compat

Copy link
Member

Choose a reason for hiding this comment

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

Why needed for Windows compat? Do you know of Path.as_posix()?

"type": "string",
"required": True,
}
ActionParameter(
Copy link
Member Author

Choose a reason for hiding this comment

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

type fix

],
output_type="list[str]",
)
async def list_files(agent, task_id: str, path: str) -> List[str]:
async def list_files(agent: Agent, task_id: str, path: str) -> List[str]:
Copy link
Member Author

Choose a reason for hiding this comment

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

type hint

@@ -186,8 +188,11 @@ async def run_action(

if __name__ == "__main__":
import sys
import asyncio
Copy link
Member Author

Choose a reason for hiding this comment

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

swifty removal and replacement with more amenable action to not actually having an agent seeded. list_files requires a full agent to be able to be pulled in to actually work

@@ -231,7 +233,9 @@ async def read_webpage(
except WebDriverException as e:
# These errors are often quite long and include lots of context.
# Just grab the first line.
msg = e.msg.split("\n")[0]
msg = "An error occurred while trying to load the page"
Copy link
Member Author

Choose a reason for hiding this comment

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

type fix

Comment on lines +132 to +151
# self.workspace.write(task_id=task_id, path="output.txt", data=b"Washington D.C")

# await self.db.create_artifact(
# task_id=task_id,
# step_id=step.step_id,
# file_name="output.txt",
# relative_path="",
# agent_created=True,
# )

# step.output = "Washington D.C"

# LOG.info(
# f"\t✅ Final Step completed: {step.step_id}. \n"
# + f"Output should be placeholder text Washington D.C. You'll need to \n"
# + f"modify execute_step to include LLM behavior. Follow the tutorial "
# + f"if confused. "
# )

# return step
Copy link
Member Author

Choose a reason for hiding this comment

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

this is from the tutorial so left here, but replaced with something that "can" work. Its bad at responding though. Could really use some help on the prompts to maek this work sometimes

@@ -2,7 +2,7 @@
The Forge SDK. This is the core of the Forge. It contains the agent protocol, which is the
core of the Forge.
"""
from ..llm import chat_completion_request, create_embedding_request, transcribe_audio
# from forge.llm import chat_completion_request, create_embedding_request, transcribe_audio
Copy link
Member Author

Choose a reason for hiding this comment

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

Circular import fix.

Why do we need this file?

Comment on lines +143 to +145

Note: this is a placeholder method and must be overridden by the agent with the actions
that the agent should perform on one step. Override this method in the ForgeAgent class.
Copy link
Member Author

Choose a reason for hiding this comment

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

Pointer for users of this to look in the right place

Comment on lines +104 to +106
raise HTTPException(
status_code=500,
detail=json.dumps(
Copy link
Member Author

Choose a reason for hiding this comment

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

pydantic prefers raise over response

Comment on lines +266 to +267
page: Optional[int] = Query(default=1, ge=1),
page_size: Optional[int] = Query(default=10, ge=1, alias="pageSize"),
Copy link
Member Author

Choose a reason for hiding this comment

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

type hints

status_code=200,
media_type="application/json",
)
steps = await agent.list_steps(task_id, page or 1, page_size or 10)
Copy link
Member Author

Choose a reason for hiding this comment

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

type error fix

try:
# An empty step request represents a yes to continue command
if not step:
step = StepRequestBody(input="y")
step = StepRequestBody(name=None, input="y")
Copy link
Member Author

Choose a reason for hiding this comment

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

type error fix

Copy link

github-actions bot commented Apr 3, 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.

Pwuts
Pwuts previously approved these changes Apr 7, 2024
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.

Looks good overall

@@ -108,6 +108,7 @@ def load_prompt(self, template: str, **kwargs) -> str:
template = os.path.join(self.model, template)
if self.debug_enabled:
LOG.debug(f"Loading template: {template}")
template = template.replace("\\", "/")
Copy link
Member

Choose a reason for hiding this comment

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

Why needed for Windows compat? Do you know of Path.as_posix()?

autogpts/forge/forge/agent.py Outdated Show resolved Hide resolved
@ntindle ntindle requested a review from Pwuts April 20, 2024 03:30
@Swiftyos
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review

⏱️ Estimated effort to review [1-5]

4, because the PR includes a wide range of changes across multiple files and functionalities, including updates to action definitions, file operations, and agent behavior. The complexity and breadth of these changes require a thorough review to ensure that all modifications are correct and do not introduce new issues.

🧪 Relevant tests

No

🔍 Possible issues

Possible Bug: The new agent loop might not handle exceptions properly, leading to unhandled exceptions if the JSON response from chat_completion_request is malformed or if the action execution fails.

Performance Concern: The use of synchronous file operations in an asynchronous context could lead to blocking behavior and reduce the efficiency of the application.

🔒 Security concerns

No

Code feedback:
relevant fileautogpts/forge/forge/__main__.py
suggestion      

Consider removing the placeholder ASCII art added in the main file, as it does not contribute to the functionality and clutters the codebase. [important]

relevant line

relevant fileautogpts/forge/forge/actions/file_system/files.py
suggestion      

Replace the synchronous file operations with asynchronous versions to avoid blocking the event loop in an async environment. This change will improve the performance and responsiveness of the application. [important]

relevant linereturn agent.workspace.list(task_id=task_id, path=str(path))

relevant fileautogpts/forge/forge/agent.py
suggestion      

Implement error handling for JSON parsing and action execution within the execute_step method to prevent the agent from crashing due to unexpected exceptions. [important]

relevant linechat_response = await chat_completion_request(**chat_completion_kwargs)

relevant fileautogpts/forge/forge/actions/registry.py
suggestion      

Ensure cross-platform compatibility by handling path separators correctly when registering actions. The current replacement might not cover all edge cases. [medium]

relevant line.replace("\\", "/")


✨ 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.

@Pwuts Pwuts changed the title Update the forge to have a very very bad, but "functional" agent by default feat(forge): Implement a very very bad, but "functional" agent by default May 3, 2024
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.

Application crashes when the LLM picks a non-existent command, which is also why it fails CI

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

4 participants