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

Use int for CandidateCount #4

Closed
rakyll opened this issue Dec 13, 2024 · 7 comments
Closed

Use int for CandidateCount #4

rakyll opened this issue Dec 13, 2024 · 7 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@rakyll
Copy link

rakyll commented Dec 13, 2024

It's very likely that users will hardcode the candidate count at a call site such as:

s.client.Models.GenerateContentStream(
     stream.Context(),
     s.config.Model,
     parts,
     &genai.GenerateContentConfig{CandidateCount: genai.Ptr(int64(5))})

In Go, it's more idiomatic to rely on int especially when it's clear that you don't need 64-bits. If candidate count was an int, it feels more natural to write without having to cast to int64:

s.client.Models.GenerateContentStream(
     stream.Context(),
     s.config.Model,
     parts,
     &genai.GenerateContentConfig{CandidateCount: genai.Ptr(5)})
@rakyll rakyll added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Dec 13, 2024
@rakyll
Copy link
Author

rakyll commented Dec 13, 2024

We can omit the use of pointers for this field, and make it look like:

type GenerateContentConfig struct {
        ...
	CandidateCount int  `json:"candidateCount,omitempty"`
	...
}

@happy-qiao
Copy link
Collaborator

I'm on it! Just need to fix some testing magic first.

@eliben
Copy link
Collaborator

eliben commented Dec 13, 2024

We can omit the use of pointers for this field, and make it look like:

type GenerateContentConfig struct {
        ...
	CandidateCount int  `json:"candidateCount,omitempty"`
	...
}

What is the meaning of a default value (0) on CandidateCount?

@happy-qiao
Copy link
Collaborator

Depends on Backend. GoogleAI throws 400 error but VertexAI return 1 candidate response

@eliben
Copy link
Collaborator

eliben commented Dec 16, 2024

There should be consistent behavior for the user here, I think. We should be careful to distinguish between "not set" and "explicitly asked 0" -- the latter can result in an error, for example (*). Therefore pointers could definitely be useful here.

(*) An error is always a good start, because it can be removed later and replaced by different behavior. If we start with different behavior, turning it into an error later is impossible if we want to retain backwards compatibility.

@happy-qiao
Copy link
Collaborator

How about using value type for candidate_count and overwrite the default 0 to 1? This can ensure the same behavior for the two backends as well as a smooth user journey.

@happy-qiao
Copy link
Collaborator

CandidateCount is now value type. When CandidateCount is zero value, the SDK will overwrite the value to 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

3 participants