[Mics]skip accelerator class getting if not name and policy specified#512
Conversation
pallasathena92
requested review from
CatherineSue,
YouNeedCryDear,
beiguo218 and
slin1237
as code owners
github-actions
Bot
added
inferenceservice
Summary of ChangesHello @pallasathena92, 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 refines the AcceleratorClass selection mechanism to provide users with more explicit control. It removes the previous implicit default behavior where an accelerator class would be automatically selected if no name or policy was provided. Now, if neither an explicit name nor a selection policy is configured, no AcceleratorClass will be applied, allowing users to opt out of accelerator class selection entirely. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully updates the accelerator class selection logic to be an opt-in feature, which is a great improvement for users who want to manage this behavior explicitly. The changes are well-implemented by removing the default policy and refactoring the selection logic for better clarity. My review includes a few suggestions to enhance logging consistency and improve a code comment for better maintainability.
|
|
||
| // getAcceleratorPolicy determines the effective AcceleratorSelectionPolicy for the given component and InferenceService | ||
| // defaulting to the selector's configured default policy if none is specified | ||
| // if none is specified, will skip getAcceleratorClassByPolicy |
There was a problem hiding this comment.
This comment describes the behavior of the calling function, not getAcceleratorPolicy itself. It would be clearer to describe what this function does when no policy is found.
A more accurate comment would be:
// If no policy is specified, an empty string is returned.
| // if none is specified, will skip getAcceleratorClassByPolicy | |
| // If no policy is specified, an empty string is returned. |
| return reconcile.Result{}, err | ||
| } | ||
| if engineAC == nil { | ||
| r.Log.Info("Accelerator class not specified for engine component", "Name", isvc.Name) |
There was a problem hiding this comment.
For consistency with other log statements in this file (e.g., line 299), it's better to use "inferenceService" as the key for the inference service name instead of "Name".
| r.Log.Info("Accelerator class not specified for engine component", "Name", isvc.Name) | |
| r.Log.Info("Accelerator class not specified for engine component", "inferenceService", isvc.Name) |
| return reconcile.Result{}, err | ||
| } | ||
| if decoderAC == nil { | ||
| r.Log.Info("Accelerator class not specified for decoder component", "Name", isvc.Name) |
There was a problem hiding this comment.
For consistency with other log statements in this file (e.g., line 330), it's better to use "inferenceService" as the key for the inference service name instead of "Name".
| r.Log.Info("Accelerator class not specified for decoder component", "Name", isvc.Name) | |
| r.Log.Info("Accelerator class not specified for decoder component", "inferenceService", isvc.Name) |
pallasathena92
force-pushed
the
yifeliu/ac-policy-remove-default
branch
from
5039574 to
d162a8e
Compare
2 participants
What this PR does
Update the logic to choose accelerator class.
Why we need it
Current behavior
AcceleratorClass (AC) selection follows this order:
Explicit AC name: If the user specifies an acceleratorClassName, the system uses that AC directly.
Policy-based selection: If no AC name is provided, the system selects an AC based on the configured policy.
Default policy: If no policy is configured, the system defaults to FirstAvailable, meaning it will still select an AC automatically.
Proposed behavior
To give customers the option to opt out of AcceleratorClass selection entirely, we change the default behavior:
Explicit AC name: If the user specifies an acceleratorClassName, the system uses that AC directly.
Policy-based selection: If no AC name is provided, the system selects an AC based on the configured policy.
No implicit default: If no policy is configured, the system does not select any AC (i.e., no AcceleratorClass is applied by default).
Fixes #
How to test
Checklist
make testpasses locally