Skip to content

Inconsistency in the use of Tool and AssistantTool #134

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

Closed
sashirestela opened this issue May 16, 2024 Discussed in #131 · 1 comment · Fixed by #140
Closed

Inconsistency in the use of Tool and AssistantTool #134

sashirestela opened this issue May 16, 2024 Discussed in #131 · 1 comment · Fixed by #140
Assignees
Labels
bug Something isn't working

Comments

@sashirestela
Copy link
Owner

Discussed in #131

Originally posted by sashirestela May 15, 2024
@the-gigi I've copied your comment to this new discussion thread:

One more issue came up as a result of the refactoring. There is inconsistency in the use of Tool and AssistantTool.
For example, AssistantRequest requires Tool:

public class AssistantRequest {
    ...
    private List<Tool> tools;
    ...
}

While AssistantModifyRequest requires AssistantTool

public class AssistantModifyRequest {
    ...
    private List<AssistantTool> tools;
    ...
}

AssistantTool extends Tool. Looking at the code it seems that all AssistantTool adds is the pre-defined tools CODE_INTERPRETER and FILE_SEARCH. Is it necessary to actually instantiate AssistantTool objects?

The idea of the refactor to a common tool is that the Tool functionality is the same between chat and assistants. The fact that assistants have pre-defined tool doesn't require a new type of AssstantTool.

Consider replacing all references to AssistantTool with just Tool. The AssistantTool class itself will still provide the CODE_INTEPRETER and FILE_SEARCH tools, but it will provide them as Tool instances.

This will simplify the code, make it consistent and utilize just the common Tool for all use cases of chat and assistant APIs.

I tried it locally and it and it works just fine (all the tests pass and all the demos).

@sashirestela sashirestela added the bug Something isn't working label May 16, 2024
@sashirestela
Copy link
Owner Author

I will implement your proposal.

@sashirestela sashirestela linked a pull request May 25, 2024 that will close this issue
@sashirestela sashirestela self-assigned this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant