-
Notifications
You must be signed in to change notification settings - Fork 676
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: unify token_limit property #1071
Conversation
Do you think it's better to directly move the default value of token limit (4096) to the |
If we move this under |
I feel like it's okay to give warning in the UnifiedModelType, as the token_limit inside is a pre-set default value, and it will only be called when a user tries to get token limit for an unrecognized str |
what is user passed an unrecognized str but set the token value in |
Then In our code I think the token_limit property will not be accessed given that the value in config dict is not None? |
@WHALEEYE updated |
Is |
Should we set it to a very large number instead? |
great point, updated |
Description
if string value is passed to
model_type
when calling commercial model platforms like Mistral then the token_limit value would missingalso update OPENAI_COMPATIBLE_MODEL example and doc
Motivation and Context
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!