Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API Shift/Refactor #93
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
API Shift/Refactor #93
Changes from 9 commits
92eabce
0ba19de
e74e6ed
e60c93b
6940a62
4696bc6
fd51839
f0185a2
232fef9
12226d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
I don't hate this name, but it always feels a bit funny to have to spell out "Default". Would something like "Moderate" or "Normal" work better here? Maybe there are some similar terms we can use as a reference? For example Kubernetes Pods have the following QoS classes:
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.
@JeremyOT if you have thoughts on what other names we can use for "Default". Another suggestion is "Standard"? I can also get behind "Moderate" or "Normal".
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.
Moderate
sounds good to me here, seems at least better thanDefault
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.
I actually like Default, because we are really trying to communicate that this is what you get when unset. No docs necessary to understand that. Something like Moderate is fine, but I still need to remember that Moderate is the default. Normal is more clear.
I don't think K8s QoS is a great comparison, since there's so much difference between behaviors of each class. It's not just more/less.
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.
What about representing these in numbers like priorities instead of abstract names? Everything is relative to each other.
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.
I think defining a discrete value is sufficient, easier for users to work with (pre-defined options to select from) and makes provider implementations easier.
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.
I can get on board with starting with 3 simple values here, but likely worth revisiting in the future to see if additional granularity would be helpful. @terrytangyuan did you have any specific use cases in mind that would benefit from having additional levels here?
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.
I wonder if
TargetModel
is a bit misleading if it also represents an adapter.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.
It does, but this mirrors the Open AI API spec, which just expects a
modelName
. So we chose not to deviate from that pattern.LMKWYT
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.
it may not be necessarily be an adapter too