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

[BREAKING_CHANGES] ChatCompletionRequest in chat.go, chagne data type float32 to float64 #517

Closed
wants to merge 1 commit into from

Conversation

pinerchen
Copy link

@pinerchen pinerchen commented Nov 2, 2023

Solve Issue: #9

Describe the change

  1. ChatCompletionRequest struct attributes annotation with omitempty treats value 0 as not set, for example temperature, top_p attribute. And this will trigger OpenAI use default value 1, temperature attribute specifically. OpenAI Chat Temperature Doc

  2. Data type float32 is not competent for some DBs like MongoDB data type. The only two options to store float data in MongoDB are Double or Decimal128, either way will arise error of "float64 can only be truncated to an integer type when truncation is enable" and "cannot decode 128-bit decimal into a float32 or float64 type" respectively in Golang float32.

Describe your solution

  1. remove some attributes' omitempty tag in ChatCompletionRequest.
  2. Change float32 to float64.

Tests
In MongoDB document, set attribute value data type as Double and temperature value to 0 or 0.01, won't cause error.

Change data type from float32 to float64 so that when reading MongoDB `Double` won't arise truncate precision issue.
@sashabaranov
Copy link
Owner

@pinerchen thank you for the PR!

I don't think removing omitempty is the way to go with this issue. I have hopes about encoding/json/v2 package, though (see #9 (comment)).

I also have a hard time agreeing with float32 to float64 conversion as the argument is very ad hoc. By that logic, we would need to convert every float32 to float64 due to some unrelated database limitation, which doesn't make sense. (e.g., it would blow up embedding size by a factor of two).

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.

2 participants