Models, Files and Fine-Tuning APIs #20
Conversation
thehappydinoa
changed the title
![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.
@thehappydinoa you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
PullRequest Breakdown
Reviewable lines of change
+ 1,357
- 664
76% Go
23% Go (tests)
2% Ignore ListType of change
Feature - These changes are adding a new feature or improvement to existing code.
| 1 Message | |
|---|---|
| Due to its size, this pull request will be reviewed by PullRequest staff before being sent to the reviewer network, and the team will reach out if there are any concerns. After this pull request is sent to the network, please be aware that it will likely have a longer turnaround time and require multiple passes from our reviewers. |
There was a problem hiding this comment.
I've reviewed the first part of this PR and didn't find anything that needs to be addressed. I'll have to come back to it and review the rest later today.
Reviewed with ❤️ by PullRequest
|
Thank you, I really appreciate it! |
There was a problem hiding this comment.
So, after further review I think there are some fundamental changes I would suggest.
I think the ClientOption function type is a little awkward and is maybe a case of engineering for problems you don't have yet. Most API Client libraries have a fixed set of options with their own getters and setters.
Your method naming is confusing at times and could be more explicit so that a developer knows what something does by looking at the name. GetFiles() vs. Files() for example.
I found this example of a well written golang API client library. It covers some basics about configuration and structure.
Reviewed with ❤️ by PullRequest
|
Understood. Just for context I didn't actually create the name scheme, I just copied in the one from gpt3.go. The same goes for the base client functionality. I am open to changing some of those, but I wanted to clarify if this MR is the right place. My main goal was to enable folks to use the new completion and fine tune endpoints. Please let me know how we should proceed, I would love to be able to use some of these features. Also maybe @tylermann can weigh in here? |
Yeah I would prefer as minimal changes as possible to the existing APIs since this is a publicly used/open source library and will break when people upgrade. Andy (pullrequest), would you mind taking another pass at this review with the mindset of preserving existing functionality? I believe the main goal here is to fulfill this issue: #12 |
There was a problem hiding this comment.
@thehappydinoa , first off thank you for the work here to improve this library and update these APIs!
I am slightly worried about the size of this PR just because it does change a lot of lines at once even if many of them are just moved from other files. Perhaps we could split those things up next time if we want to reorder and do that in a separate PR so it easier to follow the actual changes.
I know there are some intentional breaking changes as a part of this PR as we are removing some deprecated methods. Although ideally we can try to preserve as much functionality as possible especially for the most likely used methods like the Completion ones.
I left some comments inline, but I'll also play around with this to help manually test it.
|
Hey @tylermann thank you for taking the time to review my pr. Definitely something that should've split up. I will go your comments and try to implement what I can! Thanks again! |
There was a problem hiding this comment.
Understood. Just for context I didn't actually create the name scheme, I just copied in the one from gpt3.go. The same goes for the base client functionality. I am open to changing some of those, but I wanted to clarify if this MR is the right place. My main goal was to enable folks to use the new completion and fine tune endpoints. Please let me know how we should proceed, I would love to be able to use some of these features.
Also maybe @tylermann can weigh in here?
Yeah I would prefer as minimal changes as possible to the existing APIs since this is a publicly used/open source library and will break when people upgrade. Andy (pullrequest), would you mind taking another pass at this review with the mindset of preserving existing functionality? I believe the main goal here is to fulfill this issue: #12
Understood. It was just a suggestion but it makes sense to keep things as is for compatibility reasons.
|
Hey there, Thanks again, |
There was a problem hiding this comment.
thanks for the updates here @thehappydinoa , I was testing this branch locally and ran into a few errors. I will keep playing around with it but I think this current issue is preventing even the happy path for Completion requests from working for me.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
I was testing out the changes on this branch locally and was running into errors, that I believe are from this line. I don't think we used to do this defer/body close from the performRequest happy path, so it was giving me an error when trying to run go run cmd/test/main.go
invalid json response: http2: response body closed
There was a problem hiding this comment.
Ah let me fix that, I believe I was told to move it there, does this occur when there is no response body?
There was a problem hiding this comment.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.
Refactored each API into it's own file and added three new APIs. I also removed the
EnginesAPI as it is now deprecated.Changes