-
Notifications
You must be signed in to change notification settings - Fork 4
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
Parse OpenAI response embedding to contain all embeddings #150
Conversation
#[Test] | ||
public function itConvertsAResponseToAVectorResponse(): void | ||
{ | ||
$response = $this->createStub(ResponseInterface::class); |
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.
Why not using a JsonMockResponse directly?
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.
Do you maybe have more information to get me on your level here? I am a bit stuck with that you could mean with a JsonMockResponse
🤔
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.
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.
Ah! Ok. Nice one. But what would be the benefit from not mocking the ResponseInterface
here? The only thing i get from the documentation is that it would improve testing timeouts and chunked responses, which i do not want to do.
But nethertheless i have tried it and got the following exception that hints me to utilize the mock http client but i do not utilize any http client for my test as i am testing the response converter itself.
Symfony\Component\HttpClient\Exception\InvalidArgumentException: MockResponse instances must be issued by MockHttpClient before processing.
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.
i think in this case the benefit is not really there yet - more like a question of style, e.g. not using mock frameworks but implementations.
in general i can think of some infrastructure for MockResponse from file where also processing is done already - when oop design is done, it could be a quite common scenario to test those response converters and contract is ResponseInterface
yup
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.
Yes, i guess that was just a short coming - thanks for the patch & test!
When i request the embeddings with mutliple texts the response only contaion the first entry of the response. I did not really understand why it should be this way as the
VectorResponse
already supports having multiple vectors and so have adapted the response converter for the OpenAi Bridge embeddings.The response of such a request, utilizing the OpenAI embeddings is an array with a single text or an array of text. So it does not change the current result of the
VectorResponse
for the user of the request.Hope this fits?