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

fix: set topP from config #1053

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

muscionig
Copy link
Contributor

@muscionig muscionig commented Apr 4, 2024

Closes #1052

📑 Description

This PR allows for setting topP from the config yaml for the openai provider.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

I took the liberty of opening both the issue and the PR fixing it, please let me know if I should follow any other process.

@muscionig muscionig requested review from a team as code owners April 4, 2024 02:09
@AlexsJones
Copy link
Member

Thanks for opening a PR, I think for this to work though, you'll need to update the command/auth with a new flag for topP and link it into the configuration object

@muscionig
Copy link
Contributor Author

muscionig commented Apr 4, 2024

@AlexsJones thanks for the comment. That flag is already there in the auth command and if --topp is passed it is properly saved in the config yaml. However, because of the const value in the openai code, that flag is never set properly, which is the reason why I opened this as a fix and not an enhancement.

With the edits in this PR, that parameter is properly set from the config if the user provides it.

pkg/ai/openai.go Outdated Show resolved Hide resolved
@arbreezy
Copy link
Member

@AlexsJones thanks for the comment. That flag is already there in the auth command and if --topp is passed it is properly saved in the config yaml. However, because of the const value in the openai code, that flag is never set properly, which is the reason why I opened this as a fix and not an enhancement.

With the edits in this PR, that parameter is properly set from the config if the user provides it.

You are right, probably we never removed the constant when introducing the topP as a cli argument.

Could you please, also add it as an env variable in server's cmd file so we can use it in the operator?

@muscionig
Copy link
Contributor Author

@arbreezy thanks for reviewing this. I took care of the format and I have also enabled the setting of topP in the operator, I followed the way temperature is set. For the default value, I did not alter the previous behavior (which is also the recommended use from openai and other providers).

@muscionig muscionig requested a review from arbreezy April 16, 2024 00:54
@arbreezy
Copy link
Member

@arbreezy thanks for reviewing this. I took care of the format and I have also enabled the setting of topP in the operator, I followed the way temperature is set. For the default value, I did not alter the previous behavior (which is also the recommended use from openai and other providers).

It looks good, after resolving the conflicts I'll approve 👍

Signed-off-by: “Guido <muscionig@gmail.com>
Signed-off-by: “Guido <muscionig@gmail.com>
Signed-off-by: “Guido <muscionig@gmail.com>
@muscionig
Copy link
Contributor Author

@arbreezy I rebased off of main. Thanks!

@AlexsJones AlexsJones merged commit c162cc2 into k8sgpt-ai:main Apr 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG]: topP is not set from the config yaml of the openai provider
3 participants