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

omitempty option of request struct will generate incorrect request when parameter is 0. #9

Open
ArthurAmes opened this issue Aug 7, 2021 · 14 comments
Labels
bug Something isn't working

Comments

@ArthurAmes
Copy link

ArthurAmes commented Aug 7, 2021

The 'omitempty' option of the request structs should be removed. This is because it generates an incorrect request when a parameter is 0. For example, a request with the temperate field set to '0' will actually return a response as if the field had been set to one. This is because the go JSON parser does not differentiate between a 0 value and no value for float32, so Openai will receive a request without a temperate field which then defaults to a temperature value of 1.

@ArthurAmes ArthurAmes changed the title omitempty field of struct will generate incorrect request when parameter is 0. omitempty option of request struct will generate incorrect request when parameter is 0. Aug 7, 2021
@sashabaranov
Copy link
Owner

Thank you for the issue! So, with omitempty we have:

  • "Empty" (zero-valued) arguments are removed
  • OpenAI's backend uses its default for omitted arguments
  • Zeroing floats requires something like 1e-45 ≈ math.SmallestNonzeroFloat32

Without omitempty:

  • Non-specified arguments are zero-valued and present in request json
  • OpenAPI's backend does not apply its defaults (which may or may not be equal to zero-values)
  • In case OpenAPI defaults ≠ zero-values, we may have a default request value which can be tuned

I would check whether OpenAPI defaults match Go's zero-values. In case they do I think it is best to remove omitempty.

@sashabaranov
Copy link
Owner

sashabaranov commented Aug 8, 2021

Looking at CompletionRequest in completion.go it seems that OpenAPI's default parameter values do not match Go's zero-values:

type CompletionRequest struct {
	Prompt    string `json:"prompt,omitempty"`
	MaxTokens int    `json:"max_tokens,omitempty"` // OpenAPI defaults to 16

	Temperature float32 `json:"temperature,omitempty"` // OpenAPI defaults to 1
	TopP        float32 `json:"top_p,omitempty"` // OpenAPI defaults to 1

	N int `json:"n,omitempty"` // OpenAPI defaults to 1

	LogProbs int `json:"logprobs,omitempty"`

	Model *string `json:"model,omitempty"`

	Echo bool     `json:"echo,omitempty"`
	Stop []string `json:"stop,omitempty"`

	PresencePenalty  float32 `json:"presence_penalty,omitempty"` // OpenAPI defaults to 0
	FrequencyPenalty float32 `json:"frequency_penalty,omitempty"` // OpenAPI defaults to 0
	BestOf           int     `json:"best_of,omitempty"` // OpenAPI defaults to 1
}

@ArthurAmes
Copy link
Author

Perhaps it would be best to make a constructor function for the struct then? Or at least documenting the quirk. As of right now, calling the API with parameters generated from the playground will have (sometimes significantly) different results as calling the exact same parameters from a similar API in another language.

@sashabaranov
Copy link
Owner

It seems that playground's default parameters match API ones, except for max_tokens. Which parameters did you tune in order to get proper result?

@neilmadsen
Copy link

Hello,
Wanted to mention that I've just run into this problem as well.

If we don't want to get rid of the omitempty (because of different defaults), could we make the types for fields like Temperature a pointer (e.g. *float32)? That way, it will be omitted if you don't set it but it will be marshaled as 0 if you specifically set it to 0.

From reading about other people's usage of Temperature and TopP, it seems like the most common values for them are 1, 0.5, and 0. So 0 isn't an edge case. For now, I'm setting Temperature to a tiny amount to mimic 0, but that seems a little hacky.

adayNU added a commit to fabiustech/openai that referenced this issue Jan 20, 2023
Seeks to re-factor the original repo w/ many breaking changes. Major goals:

- Use more idiomatic go style. (Rename the package, accept and return pointers, shorter variable names, etc.)
- Much better documentation. Add comments to all exported types, fields, methods, packages, etc. (from OpenAI's documentation).
- Make option fields who's go default value is a valid parameter value (and also does not match the API's default) pointers, so omitempty does not strip explicitly set values. (See sashabaranov/go-openai#9 for more).
- Have a consistent style throughout. Most endpoints in the original library were implemented independently (and thus their usages feel inconsistent).
- Implement all endpoints.

Reviewer: neilmadsen
@SentientRamen12
Copy link

Hi, just another idea to resolve this: when marshalling and unmarshalling, we can set default values other than 0 by building a custom 'UnmarshalJSON' for an Options struct type.

reference: https://eli.thegreenplace.net/2020/optional-json-fields-in-go/

@fussraider
Copy link
Contributor

How do you like an option similar to how it is done in the SQL package from the GO standard library for nullable fields in the database? They just added a separate structure for such properties - NullString, NullFloat64 and etc. (example: https://cs.opensource.google/go/go/+/master:src/database/sql/sql.go;l=319).

If valid is false, then send the default value for the API - that is, 1, otherwise explicitly send the value from the corresponding property.

@SentientRamen12
Copy link

@fussraider that can also be done, we just need to make sure the marshalling and unmarshalling handles it cleanly.

@moubry
Copy link

moubry commented Jul 12, 2023

Setting the temperature at math.SmallestNonzeroFloat32 does not seem to be a sufficient workaround in my testing using openai.GPT3Dot5Turbo and CreateChatCompletion. Responses from OpenAI change on repeated requests for the same prompt, so either 1. this value isn't making it to OpenAI, 2. math.SmallestNonzeroFloat32 isn't low enough to ensure deterministic responses from OpenAI’s models, or 3. OpenAI’s models do not respond deterministically even at a zero temperature.

In my experience using the OpenAI Playground at zero temperature, it's always seemed to be deterministic for me (I don't see responses change when I submit them repeatedly like I am seeing with math.SmallestNonzeroFloat32), so my best guess is that math.SmallestNonzeroFloat32 is not a sufficient workaround for achieving zero temperature.

@neilmadsen
Copy link

In case it's helpful, OpenAI has explicitly told me in the past that (3) is the case. In real-world applications, I've seen that 0 temp becomes more non-deterministic the more tokens you have in input/output (for example, using ~32k tokens at the max means you have the highest likelihood of non-deterministic behavior even at 0 temp).

@moubry
Copy link

moubry commented Jul 12, 2023

@neilmadsen Ohhhh. You’re right. I went to OpenAI Playground with one of my longer prompts, and am seeing the same behavior I reported above in my comment — it changes even at zero temperature. Thank you for the insight.

@sashabaranov
Copy link
Owner

Seems likeencoding/json/v2 and new omitempty/omitzero might help with this issue, see golang/go#63397

@KevinColemanInc
Copy link

We design our prompt systems in Python and then re-implement them in our golang apps. This Golang SDK not matching the official Python SDK is a problem for us.

Could we either:

  1. Create a new function that requires all fields have values (and thus doesn't rely on the default openAI values)
  2. Use interfaces so functions can accept request structs without the omitempty?

sd2k added a commit to grafana/grafana-llm-app that referenced this issue May 10, 2024
This PR adds a workaround for [this GitHub discussion][gh], describing
how chat completion requests which explicitly set temperature = 0 are
marshalled to JSON incorrectly (due to the 0 being indistinguishable
from the zero value of the field).

To do so we add a custom unmarshal method which checks whether the field
was defined and set to zero, and explicitly set the temperature to
`math.SmallestNonzeroFloat32` in such cases. This isn't perfect but is
likely to get very similar results in practice.

[gh]: sashabaranov/go-openai#9 (comment)
@TuringJest
Copy link

TuringJest commented Jul 20, 2024

Been following this thread for some time and I honestly do not understand the disagreement on removing omitempty or introducing pointer fields.
Using omitempty when it conflicts with a zero-value is bad design.

Yes, the difference might be hard to measure, because the multi-agent architecture makes determinism difficult, but that's not an excuse.
Fixing this does not create more complexity than the hack which is in place now.
This library is quite popular, so it'd be great if this conflict between optional and zero-values was handled in a more expectable way.

Thanks for your work and sorry for the criticism, but every time I see this issue I just ask myself "why!?" 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants