-
Notifications
You must be signed in to change notification settings - Fork 247
[Feat] Added option to specify priority class #557
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
Conversation
Signed-off-by: Fabijan Marič Vild <[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.
Summary of Changes
Hello @Fabhiahn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new feature that enables users to specify a priorityClassName
for deployments within the Helm chart. This allows for better control over pod scheduling and resource allocation based on priority. The changes involve modifying deployment templates, updating the schema, and adding documentation to the values file.
Highlights
- Feature: Added the ability to specify
priorityClassName
for deployments, allowing users to define the priority of pods. - Deployment Configuration: Modified deployment templates for cache server, router, and vllm-multi to include an optional
priorityClassName
. - Schema Update: Updated the
values.schema.json
file to includepriorityClassName
as a valid option. - Values Configuration: Updated the
values.yaml
file to include documentation and a default value forpriorityClassName
in the router specification.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces the functionality to specify a priority class for deployments within the Helm chart. It modifies deployment templates for the cache server, router, and vllm-multi, and updates the values schema and default values to include the new priority class option. The review focuses on ensuring that the implementation is robust and handles cases where the priority class name is not provided.
{{- if $modelSpec.priorityClassName }} | ||
priorityClassName: {{ $modelSpec.priorityClassName | quote }} | ||
{{- end }} |
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.
Consider adding a check to ensure $modelSpec.priorityClassName
is not empty before quoting it. This prevents adding an empty priorityClassName
field to the deployment if the value is not provided.[^1]
{{- if and $modelSpec.priorityClassName (not (empty $modelSpec.priorityClassName)) }}
priorityClassName: {{ $modelSpec.priorityClassName | quote }}
{{- end }}
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
Signed-off-by: Fabijan Marič Vild <[email protected]> Co-authored-by: Yuhan Liu <[email protected]> Signed-off-by: Ifta Khairul Alam Adil <[email protected]>
I added an option to specify
priorityClassName
for each of the deployments.