docs: Address PR #381 review feedback - #454
Conversation
MikeSpreitzer
left a comment
There was a problem hiding this comment.
I left some individual comments.
| - **Cold start**: creating a new vLLM instance without using a launcher | ||
| - **Luke warm start**: DPC creates a new launcher pod, then the launcher creates a new vLLM instance | ||
| - **Cold start (no FMA)**: creating a new vLLM instance without using a launcher | ||
| - **Cold start (with launcher)**: DPC creates a new launcher pod, then the launcher creates a new vLLM instance |
There was a problem hiding this comment.
I still think that it is important to be clear here about the fact that the launcher does not create the vllm instance on the launcher's own initiative but rather in response to a command from the DPC. This document still reads like there is just one DPC reaction time involved, and that is just not true. For example, one bit of path not discussed is from vLLM becoming ready to the server-requesting Pod being ready. That one goes like this: the DPC occasionally asks vLLM whether it is ready; after receiving a "yes", the DPC commands the requester container in the server-requesting Pod to start responding "yes" when queried for readiness; the kubelet occasionally queries that, and upon receiving a "yes" requests a kube apiserver to set that container's readiness; some time after that, something tells the kube apiserver to mark the whole server-requesting Pod as ready.
I would replace
then the launcher creates a new vLLM instance
with
then the DPC commands the launcher to create a new vLLM instance
There was a problem hiding this comment.
Replacement is done! I appreciate the extra clarification, but I am trying to be mindful of this document's scope growing too much. In particular, I don't want to recreate the latency outline. Perhaps this doc can reference a companion doc that meets this need. I'd love to review a PR from you with doc-wide changes after this one is merged or modify this one myself once you have the latency outline completed. What do you think?
There was a problem hiding this comment.
I will come back to this after we get this one merged.
|
|
||
| Relationships: | ||
| - T_cold_launcher ≈ T_launcher_schedule + T_launcher_startup + T_dpc_react + T_instance_ready | ||
| - T_instance_create ≈ T_instance_ready (warm start; launcher already Ready, DPC react time is negligible) |
There was a problem hiding this comment.
If T_instance_create has only one constituent, why break it out?
There was a problem hiding this comment.
We break it out because collapsing it into T_instance_ready would make the metric definitions ambiguous. T_instance_create is a primary metric that applies to both warm start and cold start (with launcher) paths. T_instance_ready is a constituent of the T_cold_launcher decomposition. In the warm start case they converge, but keeping T_instance_create as the top-level metric gives warm start a named metric without forcing it into the T_cold_launcher decomposition table.
There was a problem hiding this comment.
| T_instance_ready |
CreateNamedInstancecall to DPC successfully relaying readiness to the requester pod (DPC V5 log: "Successfully relayed the readiness") | DPC logs + Kube pod status |
That does not say "warm start but not cold start" to me. Also, it is not precise about the starting instant.
There was a problem hiding this comment.
You're right on both counts. T_instance_ready applies to both cold start (with launcher) and warm start. It's a constituent of T_cold_launcher in the former, and approximately equals T_instance_create in the latter. I'll clarify this in the definition. For the starting instant, "CreateNamedInstance call" means when the DPC issues the HTTP request to the launcher. This isn't precisely observable yet (tracked in #495); the closest current proxy is the launcher's HTTP access log timestamp for the PUT request.
MikeSpreitzer
left a comment
There was a problem hiding this comment.
I left some individual comments.
| - **Luke warm start**: DPC creates a new launcher pod, then the launcher creates a new vLLM instance | ||
| - **Cold start (no FMA)**: creating a new vLLM instance without using a launcher | ||
| - **Cold start (with launcher)**: DPC creates a new launcher pod, then the launcher creates a new vLLM instance | ||
| - **Warm start**: creating a new vLLM instance in an existing launcher pod |
There was a problem hiding this comment.
There are two variants of warm start: one just creates a vllm instance, the other first deletes a sleeping instance and then creates the new instance.
The implementation does not yet do the second, but it is coming (this is Issue #246).
There was a problem hiding this comment.
Are you suggesting that we break them up into two distinct actuation paths or lump the cases but with an extended description (i.e., creating a new vLLM instance *or* delete an unsuitable sleeping instance and then create a new vLLM instance in an existing launcher pod?
There was a problem hiding this comment.
I think that maybe we could go either way. If we call these both the same "actuation path" then, for purposes of understandable results, we would need a further level of distinction --- something like different cases within one actuation path. I suspect this might be the way to go, addressing #408 is going to also split the cold-start-with-FMA actuation path into two cases. Actually both could theoretically involve more than two cases (e.g., delete 0, delete 1, and delete 2) --- but I expect that deleting more than 1 will be very rare in practice, so am fine with not benchmarking them.
There was a problem hiding this comment.
I think it makes sense to keep the lumped but insert an appropriate signal in the results metadata about which case applied (i.e., as simple as an extra label in the result string or something). I'd like to leave the implementation details for later.
…sion - Clarify end instants: T_actuation ends at kubelet readiness probe; T_cold_launcher, T_instance_create, T_instance_ready end at DPC readiness relay (V5 log: "Successfully relayed the readiness") - Rename Hit_rate to Hot_hit_rate; add Warm_hit_rate metric - Replace "negligible" with precise language for T_dpc_react in warm start - Add note that T_dpc_react excludes later DPC actions (labels/annotations) - Fix T_dpc_react observable: log not yet present, link to llm-d-incubation#495 - Add llm-d-incubation#497 link for T_instance_create start-instant observability gap - Change "launcher API responses" to "launcher logs" in preamble - Align Phase 2 descriptions with updated end-instant definitions Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
| is *high predictability*, which is defined as achieving close to 100% hit rate of awakening | ||
| available, sleeping pods on cluster GPUs as a function of total inference server | ||
| requests for common user scenarios. | ||
| is *high predictability*, which is defined as achieving close to 100% hot-start hit rate |
There was a problem hiding this comment.
Not a problem introduced in this PR, but: I see no reason to think that 100% hot-start hit rate is achievable. That would require always getting very lucky. And clearly, different usage patterns will have different hit rates.
There was a problem hiding this comment.
Hence the close part. I think it would be rare, if not impossible, to hit 100% on active clusters like ours.
There was a problem hiding this comment.
I do not think that "close to" does the job here. How good the hot-start hit rate is will depend on the overall usage pattern (over time) and how lucky is the Pod and GPU scheduling. These could easily be far from getting 100% lucky.
| - **Hot_hit_rate**: Fraction of server-requesting Pods that get satisfied by waking a sleeping vLLM instance (hot start). | ||
| - **Warm_hit_rate**: Fraction of server-requesting Pods that get satisfied by an existing launcher pod (warm start), avoiding the cost of creating a new launcher pod. | ||
| - **T_cold_launcher**: Time from (a) when the DPC sends the request to create the launcher Pod to (b) the DPC successfully relaying readiness to the requester pod (DPC V5 log: "Successfully relayed the readiness"). A constituent of T_actuation for cold start (with launcher) cases, covering launcher pod scheduling, launcher startup, and vLLM instance creation. Does not include the earlier portion of T_actuation (requester scheduling and DPC reconciliation before the launcher pod is created). | ||
| - **T_instance_create**: Time from the launcher receiving a create request ([#497](https://github.com/llm-d-incubation/llm-d-fast-model-actuation/issues/497) tracks adding subsecond launcher logging for this instant) to the DPC successfully relaying readiness to the requester pod (DPC V5 log: "Successfully relayed the readiness"). Includes the benefit of vLLM module preloading. Applies to both cold start (with launcher) and warm start paths. |
There was a problem hiding this comment.
This is not the first interval to end at readiness, but it is the first one with a remark about how that is observed. At the earlier mention I assumed that the observation plan was the last-update timestamp on the readiness Condition in the Pod's status.
The observation plan should be stated at first mention of the moment, and/or in a list of observation plans.
There was a problem hiding this comment.
I don't quite follow. What do you consider to be the remark about how it is observed? I stated it as (DPC V5 logs: ...), which is also shared with the earlier mention on T_cold_launcher. Perhaps you meant the observation for T_actuation? There, too, the readiness is quite descriptive (the kubelet's readiness probe on the requester pod succeeds, marking the Pod's Ready condition True), which aligns with your assumed observation plan about the last-update of the readiness condition.
There was a problem hiding this comment.
Yes, the earliest mention of ending at requester readiness is on new line 39 with the text "the kubelet's readiness probe on the requester pod succeeds, marking the Pod's Ready condition True". That text has two different things in it, the second one is a process (a call from the kubelet to a kube apiserver) that covers a span of time, and both are different from what is said here ("DPC V5 log ...").
| | **T_instance_ready** | `CreateNamedInstance` call to DPC successfully relaying readiness to the requester pod (DPC V5 log: "Successfully relayed the readiness") | DPC logs + Kube pod status | | ||
|
|
||
| Relationships: | ||
| - T_cold_launcher ≈ T_launcher_schedule + T_launcher_startup + T_dpc_react + T_instance_ready |
There was a problem hiding this comment.
You want to be careful about each instant involved. For example, I see two different starting instants.
T_cold_launcher says it starts when "the DPC sends the request to create the launcher Pod".
T_launcher_schedule says it starts at "Launcher pod creationTimestamp".
There was a problem hiding this comment.
Good catch. I've aligned both T_cold_launcher and T_launcher_schedule to start from the launcher Pod's creationTimestamp. The alternative would be a pre-creation DPC log statement (tracked in #495), but since Kubernetes condition timestamps (lastTransitionTime) are at second precision, using creationTimestamp (also second precision) keeps the decomposition internally consistent. The DPC-to-API-server gap is nanoseconds, so no meaningful accuracy is lost.
There was a problem hiding this comment.
The DPC-to-API-server gap is nanoseconds
Do you have evidence of that? (Beware timestamps collected from different clocks. IIRC ntp typically gets you within about 1 ms.)
But regardless of whether it is milliseconds or nanoseconds, the point about information lost is that it is some unknown fraction of a second.
There was a problem hiding this comment.
The Huygens clock synchronization work demonstrates the possibility to synchronize hosts to within tens of nanoseconds. However, I do not have such evidence for the DPC-to-API-server gap in particular.
MikeSpreitzer
left a comment
There was a problem hiding this comment.
I have finished a round of review.
…th naming Rename lukewarm start to "Cold Start (with launcher)" per reviewer feedback that the path is cold, not warm. Split Cold Start into three variants: no FMA, FMA M2 (planned), and with launcher. Rename T_luke_warm metric to T_cold_launcher. Add constituent duration metrics table (T_launcher_schedule, T_launcher_startup, T_dpc_react, T_instance_ready) as planned sub-metrics. Remove obsolete naming note. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Summary of changes: - Add Cold Start (FMA M2) to Purpose bullet list so it matches the 5-column matrix (was "four" conditions, now "different" conditions) - Clarify T_actuation: non-FMA and M2 cold starts have no FMA-specific sub-components - Drop LPC attribution from Warm Start description -- DPC does not care whether the pre-existing launcher was created by LPC or by a prior DPC reconciliation - Replace "on the correct/assigned GPU" with node-level language in Warm Start and Hot Start (launchers are on Nodes, not GPUs) - Add L2 to Resource Scaling and Stress Test (L1+L3 -> L1+L2+L3) since TTFT cost is low relative to actuation and the data is useful at scale - Remove unused L1+L3 legend entry from matrix; expand L1+L2+L3 description to show how it builds on L1+L2 - Fix Phase 2 T_launcher scope: applies to both warm and cold start with launcher, not just warm Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Drop Cold Start (FMA M2) from the actuation paths table and matrix. M2 is acknowledged as a distinct path via a note under the paths table but excluded from the benchmarking focus. This simplifies the matrix to 4 columns (no FMA, with launcher, warm, hot) and removes all "(planned)" annotations. Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
- Rename T_launcher to T_instance_create throughout: the metric measures vLLM instance creation time, not launcher behavior - Clarify T_cold_launcher is a constituent of T_actuation, not the full end-to-end path - Scope T_dpc_react to cold start (with launcher) only; in warm start the launcher is already Ready before the requester exists - Note Phase 2 T_instance_create approximation is an upper bound that includes DPC reconciliation overhead - Add alternative observability approaches: Prometheus histograms for aggregate durations, distributed tracing for per-request correlation - Clarify T_instance_create description in L1 table Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
…sion - Clarify end instants: T_actuation ends at kubelet readiness probe; T_cold_launcher, T_instance_create, T_instance_ready end at DPC readiness relay (V5 log: "Successfully relayed the readiness") - Rename Hit_rate to Hot_hit_rate; add Warm_hit_rate metric - Replace "negligible" with precise language for T_dpc_react in warm start - Add note that T_dpc_react excludes later DPC actions (labels/annotations) - Fix T_dpc_react observable: log not yet present, link to llm-d-incubation#495 - Add llm-d-incubation#497 link for T_instance_create start-instant observability gap - Change "launcher API responses" to "launcher logs" in preamble - Align Phase 2 descriptions with updated end-instant definitions Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
99522ee to
313b44a
Compare
Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Specify that T_instance_ready starts when the DPC issues the CreateNamedInstance HTTP request (with llm-d-incubation#495 tracking the needed log statement) and applies to both cold start (with launcher) and warm start paths. Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
| **Alternative observability approaches:** As an alternative to DPC log parsing for | ||
| T_dpc_react and T_instance_ready, the DPC could emit Prometheus histograms for these | ||
| durations, which would be more reliable than log parsing but only provide aggregate |
There was a problem hiding this comment.
Prometheus histograms are not strictly an alternative: they could be done additionally.
| a create request to DPC relaying readiness. Initially approximated by (requester dual-label | ||
| timestamp - launcher pod Ready timestamp), which is an upper bound that includes DPC | ||
| reconciliation overhead. A tighter measurement requires DPC log parsing or Prometheus | ||
| histograms (see alternative observability approaches above). |
There was a problem hiding this comment.
See comment on "alternative" above.
MikeSpreitzer
left a comment
There was a problem hiding this comment.
This still needs some work; I would not complain if it were done in follow-on PRs. Substantial progress has been made in this PR.
- Clarify end instants: T_actuation ends at kubelet readiness probe; T_cold_launcher, T_instance_create, T_instance_ready end at DPC readiness relay (V5 log: "Successfully relayed the readiness") - Rename Hit_rate to Hot_hit_rate; add Warm_hit_rate metric - Replace "negligible" with precise language for T_dpc_react in warm start - Add note that T_dpc_react excludes later DPC actions (labels/annotations) - Fix T_dpc_react observable: log not yet present, link to #495 - Add #497 link for T_instance_create start-instant observability gap - Change "launcher API responses" to "launcher logs" in preamble - Align Phase 2 descriptions with updated end-instant definitions Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Summary
Follow-up to #381, addressing review feedback from Mike and Ansu on the benchmarking scenarios doc.
Actuation path and naming changes
Metric precision (end-instant clarity)
Constituent metrics and observability
Other fixes
Test plan
Related issues
CreateNamedInstanceDPC log statement