-
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
Add support for various tool types #691
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.
👍 Looks good to me! Reviewed everything up to 2c8a0af in 28 seconds
More details
- Looked at
379
lines of code in5
files - Skipped
1
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/prompt_step.py:36
- Draft comment:
EnsureApiCallDef.model_fields.keys()
is the correct way to access model fields. Ifmodel_fields
is not a valid attribute, this could lead to runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
Themake_function_call
function inprompt_step.py
is usingApiCallDef.model_fields.keys()
which might not be the correct way to access model fields. This could lead to potential issues if the model fields are not accessed correctly.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:35
- Draft comment:
Usingk.rstrip("_")
to modify keys might lead to unexpected behavior if keys do not end with an underscore. Ensure this is the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%
Inprompt_step.py
, themake_function_call
function is usingk.rstrip("_")
to modify keys. This could lead to unexpected behavior if keys do not end with an underscore. It's better to ensure that keys are intended to be modified this way.
3. agents-api/agents_api/workflows/task_execution/__init__.py:173
- Draft comment:
Ensuretool_call
contains a "system" key before accessing it. If not, this could lead to issues when creatingSystemDef
. - Reason this comment was not posted:
Confidence changes required:50%
Intask_execution/__init__.py
, therun_system_call
function is usingtool_call.get("system")
. Iftool_call
does not have a "system" key, this will returnNone
, which could lead to issues when creatingSystemDef
.
4. agents-api/agents_api/workflows/task_execution/__init__.py:134
- Draft comment:
Replacingjson_
withjson
inarguments
could lead to issues ifjson_
is not intended to be replaced or ifjson
already exists. Ensure this is the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%
Intask_execution/__init__.py
, therun_api_call
function modifiesarguments
by replacingjson_
withjson
. This could lead to issues ifjson_
is not intended to be replaced or ifjson
already exists inarguments
.
Workflow ID: wflow_2ru7TQmyB3MVGjSt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
2c8a0af
to
44e4c13
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.
👍 Looks good to me! Incremental review on 44e4c13 in 33 seconds
More details
- Looked at
304
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/agents_api/common/exceptions/tasks.py:125
- Draft comment:
Unnecessary use ofcast
here. Theisinstance
check already ensures the type, makingcast
redundant. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Thecast
function is used for type hinting and does not change the runtime behavior. Since theisinstance
check already ensures thaterror
is of typehttpx.HTTPStatusError
, thecast
is indeed redundant for runtime purposes. However, it might be used for static type checking purposes. The comment is technically correct, but it might be useful for static analysis tools.
I might be overlooking the importance ofcast
for static type checking, which can be beneficial for code maintainability and readability, even if it seems redundant at runtime.
Whilecast
might be useful for static type checking, the comment is correct in pointing out that it is not necessary for runtime behavior. The decision to keep or remove thecast
should be based on the project's conventions regarding type hinting and static analysis.
The comment is correct in identifying the redundancy ofcast
after anisinstance
check. However, the decision to keep or remove thecast
depends on the project's type hinting practices. The comment should be kept as it highlights a potential simplification in the code.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:153
- Draft comment:
Ensuretools_mapping
is applied to all tool calls, not just the first one. Consider iterating overchoice.message.tool_calls
. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_1ZvqpGVze3FiWu9y
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 cbccd3e in 16 seconds
More details
- Looked at
153
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/prompt_step.py:33
- Draft comment:
Consider refactoring the repeated pattern for constructing theparameters
dictionary inmake_function_call
to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:30%
Themake_function_call
function has a repeated pattern for constructing theparameters
dictionary. This can be refactored to improve code readability and maintainability.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:89
- Draft comment:
Consider refactoring the repeated pattern for constructing theparameters
dictionary inmake_function_call
to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:30%
Themake_function_call
function has a repeated pattern for constructing theparameters
dictionary. This can be refactored to improve code readability and maintainability.
3. agents-api/agents_api/activities/task_steps/prompt_step.py:125
- Draft comment:
Consider refactoring the repeated pattern for constructing theparameters
dictionary inmake_function_call
to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:30%
Themake_function_call
function has a repeated pattern for constructing theparameters
dictionary. This can be refactored to improve code readability and maintainability.
Workflow ID: wflow_JU4Wd3NIEnEzT8Cc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
cbccd3e
to
e3e69c0
Compare
Hey @whiterabbit1983, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the new `make_function_call()` utility in `prompt_step.py` that test formatting of different tool types: 📖 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! Incremental review on e3e69c0 in 11 seconds
More details
- Looked at
153
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:33
- Draft comment:
Consider refactoring the repeated code for constructing theparameters
dictionary inmake_function_call
to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:40%
Themake_function_call
function has a lot of repeated code for constructing theparameters
dictionary. This can be refactored to improve readability and maintainability.
Workflow ID: wflow_0qC3ATDYWRTXcutp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
e3e69c0
to
4d8a55c
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 4d8a55c in 23 seconds
More details
- Looked at
153
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_dRQcbeQmYdYeAb4D
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 5fd9757 in 1 minute and 6 seconds
More details
- Looked at
335
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. README-FR.md:84
- Draft comment:
The link for 'Appels directsapi_calls
' is not correctly formatted. It should be[Appels directs
api_calls](#direct-api_calls)
to ensure proper navigation. - Reason this comment was not posted:
Marked as duplicate.
2. README-JA.md:84
- Draft comment:
The link for '直接のapi_calls
' is not correctly formatted. It should be[直接の
api_calls](#direct-api_calls)
to ensure proper navigation. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_x0fQ0NyUQpwO9APn
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.
5fd9757
to
cdd1334
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.
Skipped PR review on cdd1334 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Signed-off-by: Diwank Singh Tomer <[email protected]>
cdd1334
to
7aec8fd
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.
Skipped PR review on 7aec8fd because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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.
Skipped PR review on 3bc9911 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Closing in favor of #789 |
Important
Adds support for
api_call
,integration
, andsystem
tool types, updating workflows and dependencies accordingly.api_call
,integration
, andsystem
tool types inprompt_step.py
andtask_execution/__init__.py
.execute_system()
inexecute_system.py
to handle emptyarguments
.make_function_call()
inprompt_step.py
to format tool calls.run_api_call()
,run_integration_call()
, andrun_system_call()
intask_execution/__init__.py
to execute respective tool types.msgpack
dependency inpyproject.toml
.tasks.py
to handle HTTP errors.This description was created by for 5fd9757. It will automatically update as commits are pushed.