-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Update OpenAPIServiceConnector to new ChatMessage #8817
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 13198769367Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@richardpaulhudson would you please give this branch a spin in your project by directly installing it via |
@mpangrazzi The reason you'll see this ugly monkey patch is better described at Dorthu/openapi3#124 For better integration with LLMs handling REST endpoint responses I believe that response validation should be optional. Currently the openapi3 implementation doesn’t offer a way to disable model validation, which can sometimes lead to unintended issues and poor overall UX. In response to user feedback we'll soon deprecate OpenAPIServiceConnector for OpenAPIConnector and OpenAPITool (see internal https://github.com/deepset-ai/haystack-private/issues/104 for details). |
@dfokina I don't think there are any doc changes required here as we are not changing any public API signatures - we are just updating this component to work with the latest ChatMessage |
Just a (slightly unrelated) curiosity: |
Of course @anakin87 , shared this during brainstorming with Luke and Julian so here is the copy for the record: Difference between OpenAPIConnector and OpenAPITool
OpenAPIConnectorPros:
Cons:
OpenAPIToolPros:
Cons:
|
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! I only feel that some edge cases might be not covered by tests. For example, when request body is missing or when content type is unsupported. Do you think it may worth covering those?
Yes, good point. Let me see what I can do 🙏 |
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.
While I was looking, found a few improvements to the docstrings
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Co-authored-by: Daria Fokina <[email protected]>
Why:
Update
OpenAPIServiceConnector
to handle newChatMessage
formatOpenAPIServiceConnector
does not support the v2.9.xChatMessage
structure #8774What:
request
method in OpenAPI to handle raw responses more effectively.How can it be used:
The changes affect how
OpenAPIServiceConnector
handles newChatMessage
format involving build intool_calls
property.Example usage:
Where
message
should now containtool_calls
property.How did you test it:
Tests verify new behavior of message processing with checks for valid tool calls. Patched OpenAPI methods are mocked to assess correct invocation and response handling. Integration tests are skipped unless specific API keys are set.
Notes for the reviewer:
Pay special attention to the new patch applied to the
request
method of OpenAPI, specifically concerning raw response handling. Ensure the test coverage is sufficient for edge cases in tool call processing and response verification.