Add embeddings #11
Add embeddings #11
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@dabdine you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
This update looks good and complete. Have you considered writing any unit tests? It's good that you're using interfaces for dependencies to make it easier to mock things out for tests.
Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.
gpt3.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| output := new(EmbeddingResponse) |
There was a problem hiding this comment.
Yeah that's fair. When I write for public repos I try to copy the methodology used elsewhere in the same repository (this was almost identical to another method--though that also can just be refactored into a shared request method) so it follows a predictable tenor. I don't mind adjusting this though. It's typically how I would write it as well.
There was a problem hiding this comment.
Should be all set for both tests and reference passing.
Note: I opted to add tests using the existing methodology already in the repository. However, these tests are mostly duplicative, as they're testing the same underlying code over and over. My thought is that in a separate PR, someone can refactor the identical code blocks issuing the HTTP request for the majority of the calls, then test those methods, which would eliminate the per-API tests that don't test anything other than that common business logic.
There was a problem hiding this comment.
@tylermann We may want to consider adding this to the issues for this repo so it can be addressed in the future if its a real concern.
Yep, I'll add some tests. |
| func() (interface{}, error) { | ||
| return client.Embeddings(ctx, gpt3.EmbeddingsRequest{}) | ||
| }, | ||
| "Post \"https://api.openai.com/v1/embeddings\": request error", |
There was a problem hiding this comment.
It does not. On line 34 in gpt3_test.go, TestRequestCreationFails sets a fake transport on the http client passed to the gpt3 client. The fake transport is a FakeRoundTripper (already defined in this code base), which just mocks a transport layer, so no network sockets are opened.
However, these test cases are not testing anything unique. You can see all the same cases are just testing an error response. The error response is exactly the same except for the URL in the error message. This implies the entire set of test cases can be simplified to a single test case that tests the constructed error message meets an expected string.
This is why I claimed in a separate comment that the test cases would benefit from a refactor. In the end, the only thing different for each API call (outside of the streaming completion request) is the struct type passed to functions that are doing the same thing. There's no reason to test the golang JSON unmarshaler or HTTP client over and over (edit: the specific thing you'd want to test is that unmarshaling/marshaling to the struct type works -- that struct tags are set properly). Since all the API methods use the same or copy pasted code (except for the streaming completion endpoint), the code could be refactored, we could test the refactored/reused API call method once and have the same or better confidence that the test cases cover what we need.
The main reason why I did not pick up refactoring the code base and test cases is to keep this PR clean so it's easy to review.
There was a problem hiding this comment.
That makes sense and it is tricky testing HTTP calls, especially because things like http.NewRequest returns an error but only in cases where url.Parse() returns an error so it can be tricky to force a test to cover that.
I missed where you use the FakeRoundTripper, that's a great way to set up http client tests.
models.go
Outdated
| type EmbeddingsResponse struct { | ||
| Object string `json:"object"` | ||
| Data []EmbeddingsResult `json:"data"` | ||
| Model string `json:"model"` |
There was a problem hiding this comment.
Just to double check, is this field included in the response? I don't see this in the docs here: https://beta.openai.com/docs/api-reference/embeddings/create
There was a problem hiding this comment.
No! Great catch. Removed Model and added Usage. I updated the test cases as well.
Adds embeddings. Tested it locally with a client I have to ensure it works.