-
Notifications
You must be signed in to change notification settings - Fork 227
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
remove extraneous default params for nims that expect conservative pa… #749
Conversation
…yloads; add ability to block payload params by name
-- ideally would have liked to have done this via config, not sure it woulda been possible though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIce catch, I see a few more places we could improve code reuse.
Co-authored-by: Jeffrey Martin <[email protected]> Signed-off-by: Leon Derczynski <[email protected]>
…openaicompat default param merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Minor comment and suggestion.
@@ -38,6 +38,7 @@ class NVOpenAIChat(OpenAICompatible): | |||
"top_p": 0.7, | |||
"top_k": 0, # top_k is hard set to zero as of 24.04.30 | |||
"uri": "https://integrate.api.nvidia.com/v1/", | |||
"suppressed_params": {"n", "frequency_penalty", "presence_penalty"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a ref for these we can throw into a comment so we can keep it up to date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was learned through trial by 422 / 400 error at the moment
Co-authored-by: Erick Galinkin <[email protected]> Signed-off-by: Leon Derczynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I would like to see the new NIM test here updated to use the openai_compat_mocks
fixture. Happy to make that a follow on I will PR shortly.
resolves #748
OpenAICompatible
so we can control verbosity of payloadsNone
s from OpenAI API payloadsnim
one question - why are these defaults defined twice?