-
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
Convert api calls and integrations to tools #833
base: dev
Are you sure you want to change the base?
Conversation
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. Reviewed everything up to 193c764 in 1 minute and 27 seconds
More details
- Looked at
200
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/tests/test_activities_utils.py:16
- Draft comment:
The test usesDummyIntegrationDef
, which is commented out inget_integration_arguments
. This will always return an empty properties dictionary, potentially not testing the intended behavior. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/activities/utils.py:469
- Draft comment:
Ensureintegration_args
is a Pydantic model before accessingmodel_fields
. Otherwise, this will raise an error ifintegration_args
is not a Pydantic model. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_xSdAhlVvHFTRzZOu
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.
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 4e6eb22 in 33 seconds
More details
- Looked at
652
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/activities/utils.py:574
- Draft comment:
fld_annotation.is_required()
should befld_annotation.is_required
.is_required
is a property, not a method. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_hzvmd3SFzN1DFHGn
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.
4e6eb22
to
1e41ca5
Compare
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 0c8b30a in 26 seconds
More details
- Looked at
74
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_lXaFOmJXq8cQ5REZ
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.
@@ -255,6 +255,7 @@ model FunctionCallOption { | |||
|
|||
model NamedToolChoice { | |||
function?: FunctionCallOption; | |||
// TODO: Add integration, system, api_call |
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.
Consider updating the NamedToolChoice
model to support integration
, system
, and api_call
instead of marking them as never
. This aligns with the PR's intent to support these tool types.
@@ -288,6 +289,7 @@ | |||
type: ToolType; | |||
|
|||
function?: FunctionCallOption; | |||
// TODO: Add integration, system, api_call |
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.
Consider updating the BaseChosenToolCall
model to properly define integration
, system
, and api_call
instead of marking them as unknown
. This aligns with the PR's intent to support these tool types.
// TODO: Add integration, system, api_call | |
integration?: ChosenIntegrationCall; |
@@ -437,7 +437,7 @@ async def run( | |||
]["type"] == "integration": | |||
workflow.logger.debug("Prompt step: Received INTEGRATION tool call") | |||
|
|||
# FIXME: Implement integration tool calls | |||
# TODO: Implement integration tool calls |
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.
@whiterabbit1983 we need to implement these 3 blocks as well, refer to the ToolStep(...)
for how to do them
Important
Convert API calls and integrations to tools by updating
format_tool
and addingget_integration_arguments
, with tests for integration argument mapping.format_tool
inprompt_step.py
now supportsintegration
andapi_call
tools by setting their parameters usingget_integration_arguments
andtool.api_call.schema_
respectively.get_integration_arguments
inutils.py
to map integration providers to argument types and return their JSON schema.test_activities_utils.py
to testget_integration_arguments
with various integration definitions, ensuring correct schema generation.This description was created by for 0c8b30a. It will automatically update as commits are pushed.