-
Notifications
You must be signed in to change notification settings - Fork 900
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
fix(agents-api): Fix tool conversion issues #789
Conversation
Hey @creatorrr, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the new `get_handler` function in `utils.py` to verify it correctly maps different combinations of resource, subresource, and operation to the appropriate handler functions. 📖 For more information on how to use Sweep, please read our documentation. |
Signed-off-by: Diwank Singh Tomer <[email protected]>
5c1a073
to
6e9e8aa
Compare
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.
❌ Changes requested. Incremental review on 6e9e8aa in 43 seconds
More details
- Looked at
742
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/agents_api/activities/execute_system.py:87
- Draft comment:
Consider using a dictionary to map conditions to request types in_create_search_request
to reduce repetitive code and improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The refactoring inexecute_system.py
introduces a centralized handler retrieval usingget_handler
, which is a good practice for maintainability. However, the_create_search_request
function could be improved for clarity and maintainability by using a dictionary to map conditions to request types, reducing repetitive code.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:58
- Draft comment:
Consider adding comments to explain the logic for handlingsystem
tools usingget_handler
andtool_decorator
for better maintainability. - Reason this comment was not posted:
Confidence changes required:33%
Inprompt_step.py
, theformat_tool
function has a section for handlingsystem
tools that usesget_handler
andtool_decorator
. This is a good approach for dynamic tool handling, but the function could benefit from additional comments explaining the logic for future maintainability.
3. agents-api/agents_api/activities/utils.py:234
- Draft comment:
Consider adding a default case inget_handler
to handle unexpectedSystemDef
inputs more gracefully. - Reason this comment was not posted:
Confidence changes required:33%
Inutils.py
, theget_handler
function uses a match-case structure to return handlers based onSystemDef
. This is a clean approach, but consider adding a default case to handle unexpected inputs more gracefully.
4. agents-api/agents_api/common/protocol/tasks.py:151
- Draft comment:
Ensure all necessary fields are correctly mapped when convertingTaskToolDef
toCreateToolRequest
in thetools
property to avoid runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
Intasks.py
, thetools
property inStepContext
convertsTaskToolDef
toCreateToolRequest
. This conversion is necessary for the refactor, but ensure that all necessary fields are correctly mapped to avoid runtime errors.
5. agents-api/agents_api/workflows/task_execution/__init__.py:494
- Draft comment:
Ensure all possible tool types are covered inToolCallStep
handling to prevent unhandled cases. - Reason this comment was not posted:
Confidence changes required:50%
Intask_execution/__init__.py
, therun
method ofTaskExecutionWorkflow
handles various step types. The handling ofToolCallStep
forintegration
andapi_call
types is well-structured, but ensure that all possible tool types are covered to prevent unhandled cases.
Workflow ID: wflow_RLgl60ZuIp6rSWti
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 c281c8c in 36 seconds
More details
- Looked at
183
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. agents-api/agents_api/activities/execute_system.py:2
- Draft comment:
Callable
is used in the code but not imported. Re-add the import statement forCallable
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests that 'Callable' is used in the code, but I don't see any evidence of 'Callable' being used in the provided file content. The removal of the import seems justified as there is no usage of 'Callable'.
I might be missing some context or usage of 'Callable' that is not visible in the provided file content. However, based on the current information, there is no evidence of its usage.
Given the file content provided, there is no indication that 'Callable' is used. Therefore, the comment seems incorrect.
The comment should be deleted because there is no evidence that 'Callable' is used in the file.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:31
- Draft comment:
Use anif
statement to check for the existence of attributes before accessing them, instead of using theand
operator. This applies to lines 31, 32, 33, 34, 35, 36, 48, and 49. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The use of 'and' for conditional attribute access is a common and concise pattern in Python. The suggestion to use 'if' statements would make the code more verbose without a clear benefit. The current implementation is clear and idiomatic.
I might be overlooking potential readability improvements or specific coding standards that prefer 'if' statements over 'and'.
The current use of 'and' is a widely accepted practice in Python for such cases, and changing it to 'if' statements would unnecessarily complicate the code.
The comment should be deleted as the current use of 'and' is idiomatic and does not require a change.
3. agents-api/agents_api/activities/task_steps/prompt_step.py:221
- Draft comment:
Consider refactoring theunwrap
logic into a separate function to avoid code duplication and improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
Inprompt_step.py
, theunwrap
logic is duplicated. It would be better to refactor this logic into a separate function to avoid code duplication and improve maintainability.
4. agents-api/agents_api/activities/task_steps/prompt_step.py:247
- Draft comment:
Consider including more context in theApplicationError
message to aid in debugging. For example, include the tool name and expected tool types. - Reason this comment was not posted:
Confidence changes required:50%
Inprompt_step.py
, theApplicationError
is raised with a message that might not be informative enough for debugging. It would be helpful to include more context in the error message.
Workflow ID: wflow_jmKi7nGkEsacQ2SL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 5953d72 in 12 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/pyproject.toml:53
- Draft comment:
Ensure 'langchain-core' is compatible with existing dependencies to avoid potential conflicts. - Reason this comment was not posted:
Confidence changes required:50%
The addition of the 'langchain-core' dependency should be verified for compatibility with existing dependencies.
Workflow ID: wflow_Ihv02pfK4yNdugGf
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 4d0ee63 in 14 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/prompt_step.py:122
- Draft comment:
Use a singleget
call with a default value for better readability and efficiency.
fmt_tool.get("name", fmt_tool.get("function", {}).get("name")) : orig_tool
- Reason this comment was not posted:
Confidence changes required:50%
The code usesfmt_tool.get("name") or fmt_tool.get("function", {}).get("name")
to retrieve the tool name. This is a good approach to handle cases where the tool name might be nested under a "function" key. However, it would be more efficient to use a single call toget
with a default value, which is a more Pythonic way to handle this.
Workflow ID: wflow_rV5J2ZGkecS8muJT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 2168fe5 in 13 seconds
More details
- Looked at
28
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:238
- Draft comment:
Consider using the@beartype
decorator for all functions in this file for consistency, such asspec_to_task_data
andspec_to_task
. - Reason this comment was not posted:
Confidence changes required:50%
The use ofbeartype
decorator is inconsistent across the file. It is used fortask_to_spec
but not for other functions likespec_to_task_data
andspec_to_task
. Consistency in using type-checking decorators is important for maintainability and readability.
Workflow ID: wflow_HeU1VhuwYYN5DDlF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
❌ Changes requested. Incremental review on 0af2684 in 19 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_L8BHgXEDsdv5kEO1
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Signed-off-by: Diwank Singh Tomer [email protected]
Important
Refactor
agents-api
to improve tool conversion and system call handling by centralizing handler logic and streamlining argument processing.execute_system
inexecute_system.py
to useget_handler
for handler retrieval and streamline argument transformation.prompt_step
inprompt_step.py
to format tools usingformat_tool
and handle tool types more effectively.tool_call_step
intool_call_step.py
to construct tool calls usingconstruct_tool_call
and handle different tool types.get_handler
function inutils.py
to centralize handler retrieval logic based onSystemDef
._create_search_request
inexecute_system.py
to handle search request creation.ExecutionInput
andStepContext
intasks.py
to supportCreateToolRequest
alongsideTool
.TaskExecutionWorkflow
intask_execution/__init__.py
to handle tool calls and system calls using the refactored logic.langchain-core
dependency inpyproject.toml
.This description was created by for 0af2684. It will automatically update as commits are pushed.