feat: Add basic support for integration tools to ToolStep #519
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
![ellipsis-dev[bot]](https://github.com/avatars/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to a914e7c in 1 minute and 2 seconds
More details
- Looked at
283lines of code in3files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/autogen/Tools.py:51
- Draft comment:
Consider replacingintegration: Any | None = Nonewithintegration: IntegrationDef | None = Nonefor consistency and type safety. This applies tosystemas well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change that has already been made in the diff. Since the change is already implemented, the comment is no longer necessary. The purpose of the comment is to suggest a change for consistency and type safety, which has been addressed.
I might be missing if there are other instances in the file where the change hasn't been applied, but the comment specifically refers to the lines that have been changed.
The comment specifically refers to the lines that have been changed, and since those changes are already made, the comment is redundant.
The comment should be deleted because the suggested change has already been implemented in the diff.
2. agents-api/agents_api/autogen/Tools.py:182
- Draft comment:
Consider replacingintegration: Any | None = Nonewithintegration: IntegrationDef | None = Nonefor consistency and type safety. This applies tosystemas well. - Reason this comment was not posted:
Marked as duplicate.
3. agents-api/agents_api/autogen/Tools.py:292
- Draft comment:
Consider replacingintegration: Any | None = Nonewithintegration: IntegrationDef | None = Nonefor consistency and type safety. This applies tosystemas well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_CIKTEvwxcVKTCt3i
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6af879f in 47 seconds
More details
- Looked at
798lines of code in9files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/autogen/Common.py:16
- Draft comment:
Therootattribute inJinjaTemplateis not initialized. Consider initializing it in the constructor or setting a default value. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change that is not consistent with the rest of the file, where similar classes do not initialize 'root'. This suggests that the current pattern is intentional and likely correct. Without strong evidence that this is an issue, the comment should be removed.
I might be missing specific knowledge about the 'RootModel' class and its requirements. However, the consistency across the file suggests that the current implementation is correct.
The consistency in the file indicates that the current implementation is likely correct, and without strong evidence to the contrary, the comment should be removed.
Remove the comment as it suggests a change that is inconsistent with the rest of the file and lacks strong evidence of being an issue.
2. agents-api/agents_api/autogen/Sessions.py:46
- Draft comment:
Theforward_tool_resultsattribute is repeated across multiple classes. Consider refactoring to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
Theforward_tool_resultsattribute is repeated across multiple classes inSessions.py. This could be refactored to avoid redundancy.
3. agents-api/agents_api/autogen/Tasks.py:691
- Draft comment:
Theforward_tool_resultsattribute is repeated across multiple classes. Consider refactoring to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
Theforward_tool_resultsattribute is repeated across multiple classes inTasks.py. This could be refactored to avoid redundancy.
4. agents-api/agents_api/autogen/Tools.py:92
- Draft comment:
Theproviderattribute inIntegrationDefandIntegrationDefUpdateis well-defined usingLiteralfor valid providers. This ensures only valid providers are used. - Reason this comment was not posted:
Confidence changes required:0%
Theproviderattribute inIntegrationDefandIntegrationDefUpdateis defined as aLiteralwith specific values. This is a good practice for ensuring only valid providers are used.
Workflow ID: wflow_vAqmOM0pkSUMtc8G
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
…rovider) Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
…ions Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a49aa12 in 46 seconds
More details
- Looked at
342lines of code in6files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/migrations/migrate_1727235852_add_forward_tool_calls_option.py:15
- Draft comment:
The migration script should handle existing "auto" values in the database and convert them toNoneto maintain consistency with the updated logic in the codebase. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems speculative as it mentions 'auto' values which are not present in the script. The script already sets the new field tonull, which aligns with the comment's suggestion. The comment does not point out a clear issue with the current code.
I might be missing some context about how 'auto' values are handled elsewhere in the codebase, but based on the provided script, there is no indication of such values.
Given the script's current state, the comment does not address a specific issue in the code. It seems to be speculative and not directly relevant to the changes made.
The comment should be deleted as it does not address a specific issue in the code and seems speculative.
Workflow ID: wflow_mjimB8O79r9TAVKU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
creatorrr
changed the title
creatorrr
commented
Sep 25, 2024
| merged_tool_args = get_tool_args_from_metadata( | ||
| developer_id=developer_id, agent_id=agent_id, task_id=task_id | ||
| ) | ||
|
|
There was a problem hiding this comment.
need to also merge in integration.arguments
creatorrr
added a commit
that referenced
this pull request
Sep 30, 2024
Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com> <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > This PR updates the handling of integrations and systems by adding new models, updating workflows, and modifying session options, along with dependency updates and a migration script. > > - **Behavior**: > - Adds `execute_integration` function in `execute_integration.py` to handle integration tool calls. > - Updates `prompt_step.py` to handle unwrapping of prompt responses and tool call results. > - Modifies `tool_call_step.py` to handle tool calls using `Tool` model. > - **Models**: > - Adds `IntegrationDef` and `SystemDef` models in `Tools.py`. > - Updates `CreateToolRequest`, `PatchToolRequest`, `UpdateToolRequest`, and `Tool` to use `IntegrationDef` and `SystemDef`. > - Adds `forward_tool_results` option to session models in `Sessions.py`. > - **Workflow**: > - Updates `TaskExecutionWorkflow` in `task_execution/__init__.py` to handle integration tool calls. > - **Dependencies**: > - Updates `@typespec/*` dependencies in `package.json` to version `0.60.x`. > - **Migration**: > - Adds migration script `migrate_1727235852_add_forward_tool_calls_option.py` to add `forward_tool_calls` option to sessions. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=julep-ai%2Fjulep&utm_source=github&utm_medium=referral)<sup> for a49aa12. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Diwank Singh Tomer diwank.singh@gmail.com
Important
This PR updates the handling of integrations and systems by adding new models, updating workflows, and modifying session options, along with dependency updates and a migration script.
execute_integrationfunction inexecute_integration.pyto handle integration tool calls.prompt_step.pyto handle unwrapping of prompt responses and tool call results.tool_call_step.pyto handle tool calls usingToolmodel.IntegrationDefandSystemDefmodels inTools.py.CreateToolRequest,PatchToolRequest,UpdateToolRequest, andToolto useIntegrationDefandSystemDef.forward_tool_resultsoption to session models inSessions.py.TaskExecutionWorkflowintask_execution/__init__.pyto handle integration tool calls.@typespec/*dependencies inpackage.jsonto version0.60.x.migrate_1727235852_add_forward_tool_calls_option.pyto addforward_tool_callsoption to sessions.This description was created by
for a49aa12. It will automatically update as commits are pushed.