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 Support chat completion response format and seed new fields #525

Conversation

henomis
Copy link
Contributor

@henomis henomis commented Nov 6, 2023

Describe the change
This branch implements the support for chat completion response format optional field

Describe your solution
A new type has been added to support this new field.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #525 (5502fce) into master (d07833e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #525   +/-   ##
=======================================
  Coverage   97.59%   97.59%           
=======================================
  Files          19       19           
  Lines         872      872           
=======================================
  Hits          851      851           
  Misses         15       15           
  Partials        6        6           
Files Coverage Δ
chat.go 100.00% <ø> (ø)

@henomis henomis marked this pull request as ready for review November 7, 2023 00:30
@stillmatic
Copy link
Contributor

IMO lint should be a separate PR but this LGTM -- consider also adding seed here? It's also part of the new release, should similarly also require adding a single int parameter

https://platform.openai.com/docs/api-reference/chat/create#chat-create-seed

chat.go Outdated
Stream bool `json:"stream,omitempty"`
Stop []string `json:"stop,omitempty"`
PresencePenalty float32 `json:"presence_penalty,omitempty"`
ResponseFormat *ChatCompletionResponseFormat `json:"response_format,omitempty"`
Copy link
Sponsor Contributor

@carsonkahn-external carsonkahn-external Nov 7, 2023

Choose a reason for hiding this comment

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

Let's address seed:

Suggested change
ResponseFormat *ChatCompletionResponseFormat `json:"response_format,omitempty"`
ResponseFormat *ChatCompletionResponseFormat `json:"response_format,omitempty"`
Seed *int `json:"seed,omitempty"`

@henomis
Copy link
Contributor Author

henomis commented Nov 7, 2023

I agree @stillmatic
Thanks @carsonkahn-external

@henomis henomis changed the title Feat Support chat completion response format Feat Support chat completion response format and seed new fields Nov 7, 2023
@sashabaranov
Copy link
Owner

@henomis thank you for the PR! Could you please rebase on the latest master version so we could have clean CI runs here? (works for other PRs too!)

chat.go Outdated
Stream bool `json:"stream,omitempty"`
Stop []string `json:"stop,omitempty"`
PresencePenalty float32 `json:"presence_penalty,omitempty"`
ResponseFormat *ChatCompletionResponseFormat `json:"response_format,omitempty"`
Copy link

Choose a reason for hiding this comment

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

May not the best choice to set the type as a pointer. Should we use the value type as the other field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@sashabaranov sashabaranov merged commit 6d9c3a6 into sashabaranov:master Nov 7, 2023
3 checks passed
Stream bool `json:"stream,omitempty"`
Stop []string `json:"stop,omitempty"`
PresencePenalty float32 `json:"presence_penalty,omitempty"`
ResponseFormat ChatCompletionResponseFormat `json:"response_format,omitempty"`

Choose a reason for hiding this comment

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

This is optional. So it should have been a pointer: *ChatCompletionResponseFormat. Now I get:

status code: 400, message: '' is not one of ['json_object', 'text'] - 'response_format.type'

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.

6 participants