-
Notifications
You must be signed in to change notification settings - Fork 254
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
Make stream include usage as optional #788
Make stream include usage as optional #788
Conversation
Signed-off-by: Varun Gupta <[email protected]>
Signed-off-by: Varun Gupta <[email protected]>
After that, any feature relies on statistics won't work out of box and they have to append the stream options right? If that case, Let's update the docs as well. just an attention under feature page would be great, as well as the streaming option configuration. If you do not want to include those changes in this PR, let's create a new issue to track it |
) | ||
|
||
// validateStreamOptions validates whether stream options to include usage is set for user request | ||
func validateStreamOptions(requestID string, user utils.User, jsonMap map[string]interface{}) *extProcPb.ProcessingResponse { |
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.
How can we know user are using heterogenous features? If we have the feature flag, we can do some validation here as well. not something we need to fix in this PR, just surface this requirement
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 am thinking we should have a feature flag for heterogenous feature. Right now it is enabled by default and incurs some performance 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.
yeah, please help document this issue.
@varungup90 the change looks good to me, feel free to merge it if there's no further change |
I will create a new issue to add new feature flag to enable heterogenous features and also add support for streaming. Update documentation. |
Pull Request Description
Make stream include usage as optional parameter. If request is for a user (user has default tpm limit, if not configured) then stream's include usage is mandatory.
Heterogeneous GPU use case is not supported with stream option right now. Once the support is added, include_usage need to be enabled as well.
Related Issues
Resolves: #[Insert issue number(s)]
Important: Before submitting, please complete the description above and review the checklist below.
Contribution Guidelines (Expand for Details)
We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]
: Corrections to existing functionality[CI]
: Changes to build process or CI pipeline[Docs]
: Updates or additions to documentation[API]
: Modifications to aibrix's API or interface[CLI]
: Changes or additions to the Command Line Interface[Misc]
: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.