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

wip(agents-api): Auto-run tools in prompt steps #794

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Oct 31, 2024

Signed-off-by: Diwank Singh Tomer [email protected]


Important

Replaces forward_tool_results with auto_run_tools to control tool execution in sessions and tasks, updating models and handling various tool call types.

  • Behavior:
    • Replaces forward_tool_results with auto_run_tools in Sessions.py, Tasks.py, and typespec files.
    • auto_run_tools defaults to false for sessions and true for tasks.
    • Handles tool call types like function, integration, api_call, and system in run() in task_execution/__init__.py.
  • Models:
    • Updates CreateSessionRequest, PatchSessionRequest, Session, UpdateSessionRequest, and CreateOrUpdateSessionRequest to use auto_run_tools.
    • Updates PromptStep and PromptStepUpdateItem to use auto_run_tools.
  • Misc:
    • Excludes auto_run_tools from model_dump() in create_or_update_session.py and create_session.py.
    • Removes unused imports in execute_system.py and Tools.py.

This description was created by Ellipsis for d768c7f. It will automatically update as commits are pushed.

Copy link
Contributor

sweep-ai bot commented Oct 31, 2024

Hey @creatorrr, here is an example of how you can ask me to improve this pull request:

@Sweep Add unit tests for the new `auto_run_tools` functionality in task execution workflow, specifically testing:
- Case where `auto_run_tools=False` and tool calls are made
- Cases for different tool call types (integration, api_call, system)
- Error cases for unsupported tool call types

📖 For more information on how to use Sweep, please read our documentation.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to 387e0aa in 29 seconds

More details
  • Looked at 743 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. agents-api/agents_api/autogen/Sessions.py:47
  • Draft comment:
    Ensure that the default behavior of auto_run_tools (false for sessions, true for tasks) is clearly documented in the code comments to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from forward_tool_results to auto_run_tools is consistent across the codebase, but the default value for auto_run_tools is set to False for sessions and True for tasks. This should be clearly documented in the code comments to avoid confusion.
2. agents-api/agents_api/autogen/Tasks.py:706
  • Draft comment:
    Ensure that the default behavior of auto_run_tools (true for prompt steps, false for sessions) is clearly documented in the code comments to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from forward_tool_results to auto_run_tools is consistent across the codebase, but the default value for auto_run_tools is set to False for sessions and True for tasks. This should be clearly documented in the code comments to avoid confusion.
3. agents-api/agents_api/models/session/create_or_update_session.py:57
  • Draft comment:
    Document the reason for excluding auto_run_tools from model_dump to clarify its exclusion from session data.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The create_or_update_session function excludes auto_run_tools from model_dump, which is consistent with the intent to not include this field in the session data. This should be documented to clarify the reason for exclusion.
4. agents-api/agents_api/models/session/create_session.py:64
  • Draft comment:
    Document the reason for excluding auto_run_tools from model_dump to clarify its exclusion from session data.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The create_session function excludes auto_run_tools from model_dump, which is consistent with the intent to not include this field in the session data. This should be documented to clarify the reason for exclusion.
5. agents-api/agents_api/workflows/task_execution/__init__.py:400
  • Draft comment:
    Document the tool call types that are not yet supported and raise NotImplementedError to clarify the current limitations.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The run function in task_execution handles different tool call types but raises NotImplementedError for some. This should be clearly documented to indicate which tool call types are not yet supported.

Workflow ID: wflow_5NvTOYrgq6qb3YD3


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on d13724b in 50 seconds

More details
  • Looked at 297 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/prompt_step.py:22
  • Draft comment:
    base_evaluate is imported but not used. Consider removing it to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in agents-api/agents_api/activities/task_steps/prompt_step.py has a redundant import statement for base_evaluate. It is imported but not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:24
  • Draft comment:
    COMPUTER_USE_BETA_FLAG is defined but not used. Consider removing it if it's not needed.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In agents-api/agents_api/activities/task_steps/prompt_step.py, the COMPUTER_USE_BETA_FLAG is defined but not used anywhere in the file. This might be a leftover from previous code or a placeholder for future use. If it's not needed, it should be removed to keep the code clean.
3. agents-api/agents_api/workflows/task_execution/__init__.py:205
  • Draft comment:
    The state variable is initialized to None but is not used before reassignment. Consider removing the initialization.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In agents-api/agents_api/workflows/task_execution/__init__.py, the state variable is initialized to None but is not used before being reassigned in the match statement. This initialization is unnecessary and can be removed.
4. agents-api/agents_api/workflows/task_execution/__init__.py:377
  • Draft comment:
    Consider adding a check for finish_reason in the PromptStep case for unwrap=True to ensure correct handling.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In agents-api/agents_api/workflows/task_execution/__init__.py, the PromptStep case for unwrap=True is missing a check for finish_reason. This could lead to unexpected behavior if the finish_reason is not as expected. Consider adding a check for finish_reason to ensure correct handling.
5. agents-api/agents_api/workflows/task_execution/__init__.py:486
  • Draft comment:
    The SetStep case is duplicated. Consider removing the duplicate to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In agents-api/agents_api/workflows/task_execution/__init__.py, the SetStep case is duplicated, which is unnecessary and could lead to confusion. Consider removing the duplicate case to clean up the code.

Workflow ID: wflow_rOF5LULqhi2sb5Fl


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on d768c7f in 38 seconds

More details
  • Looked at 68 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. agents-api/agents_api/autogen/Tasks.py:686
  • Draft comment:
    Changing the default value of tools from an empty list to "all" may lead to unexpected behavior if not handled properly in the rest of the codebase. Ensure that all parts of the code that use this attribute can handle "all" as a valid value.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. integrations-service/integrations/autogen/Tasks.py:686
  • Draft comment:
    Changing the default value of tools from an empty list to "all" may lead to unexpected behavior if not handled properly in the rest of the codebase. Ensure that all parts of the code that use this attribute can handle "all" as a valid value.
  • Reason this comment was not posted:
    Marked as duplicate.
3. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:4845
  • Draft comment:
    Changing the default value of tools from an empty list to "all" in the OpenAPI spec may lead to unexpected behavior for clients. Ensure that all clients consuming this API can handle "all" as a valid default value.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_S2NYLwyjk9Ae2OqH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@creatorrr creatorrr marked this pull request as draft October 31, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant