examples: add kthena.io/model-name annotation to ModelServing samples#871
examples: add kthena.io/model-name annotation to ModelServing samples#871WHOIM1205 wants to merge 2 commits intovolcano-sh:mainfrom
Conversation
Signed-off-by: WHOIM1205 <rathourprateek8@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds the kthena.io/model-name annotation to selected ModelServing example manifests to demonstrate the recommended pattern for mapping ModelServing resources to Prometheus metrics. The annotation helps associate a ModelServing resource with its corresponding model label in metrics-based autoscaling scenarios.
Changes:
- Added
kthena.io/model-nameannotation toexamples/model-serving/sample.yamlandexamples/model-serving/data-parallel-deployment.yaml - Added the same annotation to the documentation example at
docs/kthena/docs/assets/examples/model-serving/sample.yaml - Included clarifying comment explaining the annotation's purpose
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| examples/model-serving/sample.yaml | Added kthena.io/model-name annotation with value "sample-model" |
| examples/model-serving/data-parallel-deployment.yaml | Added kthena.io/model-name annotation with value "my-model" |
| docs/kthena/docs/assets/examples/model-serving/sample.yaml | Added kthena.io/model-name annotation with value "sample-model" (documentation mirror) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| annotations: | ||
| # Identifies the model for metrics-based autoscaling (must match Prometheus "model" label). | ||
| kthena.io/model-name: "sample-model" |
There was a problem hiding this comment.
This PR adds the kthena.io/model-name annotation to some ModelServing examples but appears incomplete. Other example files in the same directory lack this annotation: gangPolicy.yaml, gpu-pd-disaggregation.yaml, multi-node.yaml, network-topology.yaml, prefill-decode-disaggregation.yaml, and rollingupdate.yaml. For consistency, all ModelServing example manifests should include this annotation to demonstrate the recommended pattern.
![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 adds the kthena.io/model-name annotation to several ModelServing example configuration files to enable metrics-based autoscaling. Feedback was provided regarding the consistency of the annotation value with the resource name in the sample.yaml files to maintain clarity across the documentation.
| namespace: default | ||
| annotations: | ||
| # Identifies the model for metrics-based autoscaling (must match Prometheus "model" label). | ||
| kthena.io/model-name: "sample-model" |
There was a problem hiding this comment.
The value of the kthena.io/model-name annotation (sample-model) is inconsistent with the resource name (sample). In other examples, such as data-parallel-deployment.yaml, the annotation value matches the resource name. For better clarity and consistency across the documentation and examples, it is recommended to use the resource name as the model name value unless there is a specific reason to demonstrate they can differ.
kthena.io/model-name: "sample"| namespace: default | ||
| annotations: | ||
| # Identifies the model for metrics-based autoscaling (must match Prometheus "model" label). | ||
| kthena.io/model-name: "sample-model" |
There was a problem hiding this comment.
The value of the kthena.io/model-name annotation (sample-model) is inconsistent with the resource name (sample). In other examples, such as data-parallel-deployment.yaml, the annotation value matches the resource name. For better clarity and consistency across the documentation and examples, it is recommended to use the resource name as the model name value unless there is a specific reason to demonstrate they can differ.
kthena.io/model-name: "sample"
|
/lgtm |
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
Wait: if this annotation requires user to set, why do we need to add to our docs
|
hey @hzxuzhonghu |
|
So actually it can be any label key, right? Please only add the label when you document autosclaing example. |
…mples Signed-off-by: WHOIM1205 <rathourprateek8@gmail.com>
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
hey @hzxuzhonghu |
|
Will there be any further updates to this PR? |
no further updates planned for this pr this was part of an earlier approach but based on feedback i moved away from adding annotations directly ive opened a new pr with the updated direction focusing on autoscaling example using user defined labels |
Summary
kthena.io/model-nameannotation to ModelServing example manifestsmodellabel in Prometheus metricsScope
Test Plan
Verified manifests apply successfully using:
kubectl apply -f examples/model-serving/sample.yaml --dry-run=clientref: Proposal: Prometheus + KEDA autoscaling for ModelServing #868
This PR implements the first step from the proposal:
Proposal: Prometheus + KEDA autoscaling for ModelServing #868
It introduces annotation-based model mapping in examples and docs.