-
Notifications
You must be signed in to change notification settings - Fork 904
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
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Diwank Singh Tomer <[email protected]>
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: 📖 For more information on how to use Sweep, please read our documentation. |
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.
👍 Looks good to me! Reviewed everything up to 387e0aa in 29 seconds
More details
- Looked at
743
lines of code in12
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 ofauto_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 fromforward_tool_results
toauto_run_tools
is consistent across the codebase, but the default value forauto_run_tools
is set toFalse
for sessions andTrue
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 ofauto_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 fromforward_tool_results
toauto_run_tools
is consistent across the codebase, but the default value forauto_run_tools
is set toFalse
for sessions andTrue
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 excludingauto_run_tools
frommodel_dump
to clarify its exclusion from session data. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_or_update_session
function excludesauto_run_tools
frommodel_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 excludingauto_run_tools
frommodel_dump
to clarify its exclusion from session data. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_session
function excludesauto_run_tools
frommodel_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 raiseNotImplementedError
to clarify the current limitations. - Reason this comment was not posted:
Confidence changes required:50%
Therun
function intask_execution
handles different tool call types but raisesNotImplementedError
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.
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
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.
👍 Looks good to me! Incremental review on d13724b in 50 seconds
More details
- Looked at
297
lines of code in7
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 inagents-api/agents_api/activities/task_steps/prompt_step.py
has a redundant import statement forbase_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%
Inagents-api/agents_api/activities/task_steps/prompt_step.py
, theCOMPUTER_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:
Thestate
variable is initialized toNone
but is not used before reassignment. Consider removing the initialization. - Reason this comment was not posted:
Confidence changes required:50%
Inagents-api/agents_api/workflows/task_execution/__init__.py
, thestate
variable is initialized toNone
but is not used before being reassigned in thematch
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 forfinish_reason
in thePromptStep
case forunwrap=True
to ensure correct handling. - Reason this comment was not posted:
Confidence changes required:50%
Inagents-api/agents_api/workflows/task_execution/__init__.py
, thePromptStep
case forunwrap=True
is missing a check forfinish_reason
. This could lead to unexpected behavior if thefinish_reason
is not as expected. Consider adding a check forfinish_reason
to ensure correct handling.
5. agents-api/agents_api/workflows/task_execution/__init__.py:486
- Draft comment:
TheSetStep
case is duplicated. Consider removing the duplicate to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
Inagents-api/agents_api/workflows/task_execution/__init__.py
, theSetStep
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.
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.
👍 Looks good to me! Incremental review on d768c7f in 38 seconds
More details
- Looked at
68
lines of code in3
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 oftools
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 oftools
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 oftools
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.
Signed-off-by: Diwank Singh Tomer [email protected]
Important
Replaces
forward_tool_results
withauto_run_tools
to control tool execution in sessions and tasks, updating models and handling various tool call types.forward_tool_results
withauto_run_tools
inSessions.py
,Tasks.py
, andtypespec
files.auto_run_tools
defaults tofalse
for sessions andtrue
for tasks.function
,integration
,api_call
, andsystem
inrun()
intask_execution/__init__.py
.CreateSessionRequest
,PatchSessionRequest
,Session
,UpdateSessionRequest
, andCreateOrUpdateSessionRequest
to useauto_run_tools
.PromptStep
andPromptStepUpdateItem
to useauto_run_tools
.auto_run_tools
frommodel_dump()
increate_or_update_session.py
andcreate_session.py
.execute_system.py
andTools.py
.This description was created by for d768c7f. It will automatically update as commits are pushed.