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

[CHAT] Make ChatGPT model ID configurable #4558

Closed
wants to merge 2 commits into from

Conversation

bowenliang123
Copy link
Contributor

Why are the changes needed?

  • Make ChatGPT model ID configurable

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@github-actions github-actions bot added kind:documentation Documentation is a feature! module:common labels Mar 19, 2023
@bowenliang123
Copy link
Contributor Author

According to OpenAI's API reference(https://platform.openai.com/docs/api-reference/completions/create), many detailed settings of chat completion request, other than model id, like max_tokens, topP, can be configured.
Do you guys think we should expose them one by one in separate config in Kyuubi , or just expose a config in JSON style, that consumes dynamic settings for ChatGPT? @pan3793 @cxzl25

@pan3793
Copy link
Member

pan3793 commented Mar 19, 2023

I would suggest just exposing the most frequently usage small set of the whole parameters.

@pan3793
Copy link
Member

pan3793 commented Mar 19, 2023

Please add this PR into the Umbrella task list

@bowenliang123
Copy link
Contributor Author

I would suggest just exposing the most frequently usage small set of the whole parameters.

Agree. And let's expose parameters on specific purpose.

@bowenliang123
Copy link
Contributor Author

Please add this PR into the Umbrella task list

Added. Thx for reminder.

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2023

Codecov Report

Merging #4558 (3012cca) into master (24e87ef) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 3012cca differs from pull request most recent head 63f8ee3. Consider uploading reports for the commit 63f8ee3 to get more accurate results

@@             Coverage Diff              @@
##             master    #4558      +/-   ##
============================================
- Coverage     53.27%   53.25%   -0.02%     
  Complexity       13       13              
============================================
  Files           573      573              
  Lines         31493    31498       +5     
  Branches       4237     4237              
============================================
- Hits          16777    16775       -2     
- Misses        13135    13140       +5     
- Partials       1581     1583       +2     
Impacted Files Coverage Δ
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 97.18% <100.00%> (+<0.01%) ⬆️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bowenliang123 bowenliang123 self-assigned this Mar 19, 2023
@bowenliang123 bowenliang123 added this to the v1.8.0 milestone Mar 19, 2023
@bowenliang123
Copy link
Contributor Author

Thanks, merged to master.

@bowenliang123 bowenliang123 deleted the chatgpt-model branch March 19, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:documentation Documentation is a feature! module:common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants