Skip to content

fix: remove mid-stream TPOT predictions from predicted-latency producer - #2037

Open
MicheleCampi wants to merge 1 commit into
llm-d:mainfrom
MicheleCampi:remove-midstream-tpot-predictions
Open

fix: remove mid-stream TPOT predictions from predicted-latency producer#2037
MicheleCampi wants to merge 1 commit into
llm-d:mainfrom
MicheleCampi:remove-midstream-tpot-predictions

Conversation

@MicheleCampi

Copy link
Copy Markdown

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes the mid-stream TPOT prediction path from the predicted-latency producer, as proposed in #2016. The sampled mid-stream prediction in processTokenForLatencyPrediction was observability-only (consumed by recordRequestPredictedTPOT, never used for routing/scoring/admission) and low quality: computed from lastSeenMetrics (the scheduling-time snapshot, not fresh state), averaged together with the scheduling-time TPOT prediction (mixing two incommensurate quantities), and the only remaining caller of buildPredictionRequest that used the scraped vLLM RunningRequestsSize gauge while training entries use the router's in-flight tracker (train/serve inconsistency, see #1856 review).

With the removal, avgPredictedTPOT reduces to the scheduling-time prediction, which is the meaningful predicted-vs-actual comparison, and the gauge/tracker inconsistency disappears by construction.

Scope notes:

  • decodeTokenSampler becomes dead code (its only remaining consumer was the removed path, since the actual avgTPOT is computed from e2e latency at end-of-stream) and is deleted along with the write-only tpotObservations field.
  • samplingMean and maxDecodeTokenSamplesForPrediction are kept in Config so existing configs keep parsing under the strict decoder, but the values are ignored; a deprecation message is logged when they are set. Their validation is removed accordingly, and the factory test covers the new accept-and-ignore behavior.
  • buildPredictionRequest itself and its remaining training-path caller are untouched to avoid conflicts with feat: consume InFlightLoadProducer in predicted-latency producer #1856.

Which issue(s) this PR fixes:
Fixes #2016

Release note:

Removed low-quality mid-stream TPOT predictions from the predicted-latency producer; avgPredictedTPOT now reflects the scheduling-time prediction only. The samplingMean and maxDecodeTokenSamplesForPrediction config parameters are deprecated and ignored.

@MicheleCampi
MicheleCampi requested review from a team and kaushikmitr as code owners July 15, 2026 20:34
@MicheleCampi
MicheleCampi requested review from elevran and vMaroon July 15, 2026 20:34
@github-actions

Copy link
Copy Markdown
Contributor

🚨 Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

@github-actions github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup labels Jul 15, 2026
@MicheleCampi
MicheleCampi force-pushed the remove-midstream-tpot-predictions branch 2 times, most recently from cd109ab to 5e7e43d Compare July 16, 2026 14:34
@ahg-g

ahg-g commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

/assign @kaushikmitr

}

// processTokenForLatencyPrediction records actual inter-token latency, sampled predictions, and advances timestamp.
// processTokenForLatencyPrediction records actual inter-token latency and advances the timestamp.

@kaushikmitr kaushikmitr Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont think this comment is correct. What we are recording is infact TPOT and not ITL

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the quantity recorded here is exactly what the codebase calls TPOT (the debug log two lines below labels it actual_tpot_ms, and it feeds the actual side of the predicted-vs-actual comparison). Reworded the comment to say TPOT. Fixed in the rebased commit.

@kaushikmitr

Copy link
Copy Markdown
Contributor

please rebase.

@MicheleCampi
MicheleCampi force-pushed the remove-midstream-tpot-predictions branch from 5e7e43d to 101419a Compare July 22, 2026 16:38
@MicheleCampi

Copy link
Copy Markdown
Author

Rebased on current main (efb4db5) — clean, no conflicts. The comment wording flagged above is fixed in the same commit; package tests pass on the rebased branch.

@kaushikmitr

Copy link
Copy Markdown
Contributor

/lgtm

@MicheleCampi

Copy link
Copy Markdown
Author

The e2e-tests failure is unrelated to this PR: the coordinator pod exits cleanly mid-test (pod dump shows llm-d-coordinator 0/1 Succeeded at 1m19s; its logs end with a graceful shutdown after ~70s of healthy readyz), and the request then times out against a backend-less service. The EPP config in that e2e doesn't instantiate the predicted-latency producer at all, and the same workflow fails intermittently on main, including on the docs-only commit this branch is rebased on (efb4db58). Could someone with access re-run it?

@kaushikmitr

Copy link
Copy Markdown
Contributor

There are commit sign failures

@MicheleCampi
MicheleCampi force-pushed the remove-midstream-tpot-predictions branch from 101419a to 3d176dc Compare July 24, 2026 16:09
@MicheleCampi

Copy link
Copy Markdown
Author

Done — the commit is now SSH-signed and shows as Verified (3d176dc). Content is byte-identical to the previously reviewed commit; I only re-signed it and dropped a leftover # Conflicts: block from the commit message.

@MicheleCampi

Copy link
Copy Markdown
Author

The three red checks on this PR are a transient GitHub infrastructure
failure from the 24 Jul run, not a code issue — and they've been sitting
there since, so I don't think anyone has looked.

All three trace back to one step in lint (make build):

  • actions/checkout download returned 429 Too Many Requests (twice)
  • kustomize download from codeload.github.com returned 504, leaving
    tar: Error is not recoverable: exiting now
  • build-epp then failed with exit 125, and docker reported
    ghcr.io/llm-d/llm-d-builder/manifests/dev: denied because the builder
    image was never produced

test and unit-tests fail downstream of the same missing image.

main is green today, so this looks like a bad window rather than
anything persistent. A re-run of CI - Lint, CI - Test and
CI - Coordinator Test should clear it — I can't trigger it myself,
since the rerun action requires org membership or an ok-to-test label
and this PR has neither.

Content is unchanged since the /lgtm on 22 Jul. The PR is still the
single commit it always was — 3d176dc is that same commit re-signed
with SSH as @kaushikmitr asked, and I verified the diff byte-identical
to the reviewed one before force-pushing. Check signed commits now
passes.

@kaushikmitr @ahg-g — a re-run and a fresh look would unblock this.

@kaushikmitr

Copy link
Copy Markdown
Contributor

Needs one more rebase, #2125 (merged 28 Jul) touched the mid-stream prediction block this PR removes, so the branch now conflicts with main. Resolution should be trivial: the deletion wins, and #2125's encoder-feature changes in the first-token/training paths are unaffected. Content otherwise LGTM.

@MicheleCampi
MicheleCampi force-pushed the remove-midstream-tpot-predictions branch from 3d176dc to 9163315 Compare July 30, 2026 20:28
@MicheleCampi

Copy link
Copy Markdown
Author

Rebased onto main (91633159), thanks for the pointer to #2125.

One conflict, in requestcontrol_hooks.go: main now carries the two encoder-cache arguments #2125 added to buildPredictionRequest, inside the mid-stream prediction block this PR removes. Resolved as you described — the deletion wins, so those two argument lines go with the block. #2125's encoder-cache work is untouched: plugin.go auto-merged (the new import, the encoderCacheDataKey field and the UseEncoderCacheFeatures config option are all intact), and the feature's own tests pass.

The diffstat moves from -294/+16 to -296/+16 for exactly that reason: two more lines to delete inside the removed block. git range-diff against the previously reviewed commit shows those two lines as the only change.

Local gate before pushing: go build ./..., go vet and go test on the predictedlatency package all green, including the tests #2125 added.

The three red checks from the 24 Jul run were queued behind the conflict; they are re-running now on the new head.

@kaushikmitr

kaushikmitr commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

please rebase:
This branch is out-of-date with the base branch
Merge the latest changes from main into this branch.

@kaushikmitr

Copy link
Copy Markdown
Contributor

/lgtm

The mid-stream prediction path in processTokenForLatencyPrediction was
observability-only and low quality: computed from the scheduling-time
metrics snapshot, averaged together with the scheduling-time TPOT
prediction, and the only remaining caller of buildPredictionRequest
using the scraped RunningRequestsSize gauge (train/serve inconsistency).

avgPredictedTPOT now reduces to the scheduling-time prediction, which
is the meaningful predicted-vs-actual comparison.

The decodeTokenSampler becomes dead code with this removal (its only
remaining consumer was the removed prediction path) and is deleted,
along with the write-only tpotObservations field. The samplingMean and
maxDecodeTokenSamplesForPrediction config parameters are kept in Config
for compatibility with existing configs (the strict decoder would
otherwise reject them) but are ignored, with a deprecation log when set.

Fixes llm-d#2016

Signed-off-by: Michele Campi <215741962+MicheleCampi@users.noreply.github.com>
@MicheleCampi

Copy link
Copy Markdown
Author

Rebased onto 9a8c999f — new head 4bf87dfb, signed. No conflicts this time, and git range-diff against the commit you /lgtm-ed reports the patch as identical (=), so the content you reviewed is unchanged. Diffstat stays at -296/+16 across the same 6 files.

Local gate before pushing: go build ./..., go vet and go test on the predictedlatency package all green.

Note that the force-push will have dismissed the /lgtm again — sorry for the churn.

@MicheleCampi
MicheleCampi force-pushed the remove-midstream-tpot-predictions branch from 9163315 to 4bf87df Compare July 31, 2026 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/cleanup size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove mid-stream TPOT predictions from the predicted-latency producer

3 participants