Skip to content
This repository was archived by the owner on Jul 16, 2025. It is now read-only.

Add topP support #37

Closed
wants to merge 1 commit into from
Closed

Add topP support #37

wants to merge 1 commit into from

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark self-assigned this Sep 25, 2024
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Only a general question - no hard feelings.

Comment on lines 21 to +22
private readonly float $temperature = 1.0,
private readonly float $topP = 1.0,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think in general of having those parameters here? Struggle a bit with my initial decision - should maybe only the part of the $options of call(..) method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for default settings for fine tuning this position is fitting. But maybe all settings available should be put to a config object?

When i have a look to the OpenAI Playground ... there are more settings beside temperature and topP for every request. So this would stack up to this amount.

image

Maybe this could also be utilized as argument by the user?

Copy link
Member

Choose a reason for hiding this comment

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

maintaining a config object would be hard - the api contract is growing and depends on versions and platform. we could tackle that of course with having designated extension points, but having an array is way more flexible for now 🙊 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can go with $options array then, but we couldn't validate them, just pass them to the model -> fire and forget... 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

The model/Platform will validate anyways and we're just reproducing their rules here

Copy link
Member

Choose a reason for hiding this comment

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

We need way better error handling tho for http calls. Was quite lazy with that ... 👼

) {
$this->version ??= Version::gpt4o();

Assert::greaterThanEq($temperature, 0.01);
Copy link
Member

Choose a reason for hiding this comment

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

the api also allows 0

Suggested change
Assert::greaterThanEq($temperature, 0.01);
Assert::greaterThanEq($temperature, 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my research it needs to be at least 0.01

Copy link
Member

Choose a reason for hiding this comment

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

Research meaning usage? Pretty sure i used 0 before but can check tomorrow or later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanShot 2024-09-25 at 22 09 11@2x

Yes you are right, based on the docs, however, when I played with the assistants:
CleanShot 2024-09-25 at 22 10 33@2x

You cannot go below 0.01

Copy link
Member

Choose a reason for hiding this comment

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

UI says no, API says yes

image

image

@OskarStark
Copy link
Contributor Author

Superseded by

@OskarStark OskarStark closed this Sep 25, 2024
@chr-hertel chr-hertel deleted the feature/top-p branch September 25, 2024 20:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants