Skip to content
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

feat: Add basic support for integration tools to ToolStep #519

Merged
merged 7 commits into from
Sep 25, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Sep 24, 2024

Signed-off-by: Diwank Singh Tomer [email protected]


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.

This description was created by Ellipsis for a49aa12. It will automatically update as commits are pushed.

Signed-off-by: Diwank Singh Tomer <[email protected]>
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 a914e7c in 1 minute and 2 seconds

More details
  • Looked at 283 lines of code in 3 files
  • Skipped 1 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. agents-api/agents_api/autogen/Tools.py:51
  • Draft comment:
    Consider replacing integration: Any | None = None with integration: IntegrationDef | None = None for consistency and type safety. This applies to system as 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 replacing integration: Any | None = None with integration: IntegrationDef | None = None for consistency and type safety. This applies to system as well.
  • Reason this comment was not posted:
    Marked as duplicate.
3. agents-api/agents_api/autogen/Tools.py:292
  • Draft comment:
    Consider replacing integration: Any | None = None with integration: IntegrationDef | None = None for consistency and type safety. This applies to system as 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 6af879f in 47 seconds

More details
  • Looked at 798 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. agents-api/agents_api/autogen/Common.py:16
  • Draft comment:
    The root attribute in JinjaTemplate is 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:
    The forward_tool_results attribute is repeated across multiple classes. Consider refactoring to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The forward_tool_results attribute is repeated across multiple classes in Sessions.py. This could be refactored to avoid redundancy.
3. agents-api/agents_api/autogen/Tasks.py:691
  • Draft comment:
    The forward_tool_results attribute is repeated across multiple classes. Consider refactoring to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The forward_tool_results attribute is repeated across multiple classes in Tasks.py. This could be refactored to avoid redundancy.
4. agents-api/agents_api/autogen/Tools.py:92
  • Draft comment:
    The provider attribute in IntegrationDef and IntegrationDefUpdate is well-defined using Literal for valid providers. This ensures only valid providers are used.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The provider attribute in IntegrationDef and IntegrationDefUpdate is defined as a Literal with 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 a49aa12 in 46 seconds

More details
  • Looked at 342 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted 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 to None to 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 to null, 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 creatorrr changed the title feat: Update typespec for integrations feat: Add basic support for integration tools to ToolStep Sep 25, 2024
@creatorrr creatorrr merged commit a4151d0 into dev Sep 25, 2024
4 of 7 checks passed
@creatorrr creatorrr deleted the f/integrations branch September 25, 2024 14:08
merged_tool_args = get_tool_args_from_metadata(
developer_id=developer_id, agent_id=agent_id, task_id=task_id
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <[email protected]>

<!-- 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants