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: Tool openapi integration #151

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Dec 9, 2024

Why:

Creates Tool instances from OpenAPI specifications, enabling the invocation of tools based on OpenAPI JSON or YAML formats.

What:

  • Introduced the from_openapi_spec class method in the Tool class, enabling tool creation from OpenAPI specifications provided as URLs, file paths, or string content.
  • Added OpenAPIKwargs TypedDict to encapsulate configuration options for OpenAPI integration, including API credentials and allowed operations.
  • Uses https://github.com/vblagoje/openapi-llm/ library that is essentially eviscerated code from the former OpenAPITool

How can it be used:

  • To create tools with OpenAPI specifications:
from haystack_experimental.components.tools.openapi import create_tools_from_openapi_spec

tools = create_tools_from_openapi_spec(
            spec="https://bit.ly/serperdev_openapi",
            credentials="your-api-key-here"
)
# now you can use tools just like any other Haystack tool
  • Supports various methods for importing OpenAPI specifications, making it easy to manage different sources of API definitions.
  • Ensures customizable invocation of API endpoints through dynamically generated tool definitions based on the OpenAPI schema.

How did you test it:

  • Conducted unit tests that validate the from_openapi_spec method's functionality and verify that tool attributes align with the OpenAPI definitions.
  • See the notebook
  • More tests soon to be added

Notes for the reviewer:

  • Pay special attention to the implementation of the from_openapi_spec method to ensure comprehensive handling of various input types (URLs, file paths, string content).

@coveralls
Copy link

coveralls commented Dec 9, 2024

Pull Request Test Coverage Report for Build 12434882747

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 83.267%

Totals Coverage Status
Change from base Build 12422960154: 0.07%
Covered Lines: 2095
Relevant Lines: 2516

💛 - Coveralls

@vblagoje
Copy link
Member Author

vblagoje commented Dec 9, 2024

Before I add more tests @anakin87 please have a look to see if you like the general direction of this approach. I've designed it to require minimal plug-in point to our new Tooling cc @julian-risch @Amnah199

@vblagoje vblagoje requested a review from anakin87 December 9, 2024 22:24
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

In general, I like this lightweight approach (mostly due to the fact that we are moving the logic to an external library).

Before moving on, I'd like @julian-risch to take a look.

Comment on lines 23 to 26
with LazyImport(message="Run 'pip install openapi-llm'") as openapi_llm_import:
from openapi_llm.client.config import ClientConfig
from openapi_llm.client.openapi import OpenAPIClient
from openapi_llm.core.spec import OpenAPISpecification
Copy link
Member

Choose a reason for hiding this comment

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

In general, I see that offloading most of the logic in Vladimir's library results for us in less maintenance effort (and I am happy about that).
Nevertheless, I see some risks. @julian-risch what's your take on this aspect?

Comment on lines 210 to 211
@classmethod
def from_openapi_spec(cls, spec: Union[str, Path], **kwargs: OpenAPIKwargs) -> List["Tool"]:
Copy link
Member

Choose a reason for hiding this comment

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

Differently from @from_function class method, this method returns a list of Tools.
Calling Tool.from_openapi_spec and getting a list of Tools seems a bit confusing from a semantic point of view.

Maybe is it better to create a function (not a class method): something like create_tools_from_openapi_spec.
WDYT? Do you have better ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point. I also see a separatecreate_tools_from_openapi_specfunction as a viable alternative but let me see if there is even better approach.

@@ -237,3 +327,41 @@ def deserialize_tools_inplace(data: Dict[str, Any], key: str = "tools"):
deserialized_tools.append(Tool.from_dict(tool))

data[key] = deserialized_tools


def _standardize_tool_definition(llm_specific_tool_def: Dict[str, Any]) -> Optional[Dict[str, Any]]:
Copy link
Member

Choose a reason for hiding this comment

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

This utility function only ensures compatibility between Haystack and the new library. I don't expect to use it elsewhere in Haystack if we do things right.
Could it be moved to the library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll move it.

@vblagoje
Copy link
Member Author

Refactored it @anakin87 @julian-risch

:returns: Tool instance for the specified operation
:raises ValueError: If the OpenAPI specification is invalid or cannot be loaded
"""
from haystack_experimental.tools.openapi import _create_tool_from_openapi_spec
Copy link
Member Author

Choose a reason for hiding this comment

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

Circular dep between these two otherwise

@vblagoje
Copy link
Member Author

Here is another idea to think about if we are to follow the Tool.from pattern. This method requires openapi spec and operation name and creates only one Tool just like other from methods. Just an idea....

@vblagoje vblagoje marked this pull request as ready for review December 20, 2024 16:24
@vblagoje vblagoje requested a review from a team as a code owner December 20, 2024 16:24
@vblagoje vblagoje requested review from Amnah199 and removed request for a team December 20, 2024 16:24
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.

3 participants