Repository navigation
fix(observability): restore speculative-decoding spans on the Spec V2 paths - #37352
MicheleCampi wants to merge 1 commit into
Conversation
f98e784 to
b9e29af
Compare
|
@hnyls2002 @Qiaolin-Yu — this needs the I rebased it onto I could not run the test locally (no @RichardoMrMu — this restores the stages you added in #19545, on the paths you reported in #37128. |
|
Thanks for picking this up — I filed #37128 and went through this PR against all three gaps in that report; it looks complete and correct to me:
One detail worth calling out: the LGTM from a reporter's standpoint. Would be great to get a maintainer look (cc @sufeng-buaa) so this can land. |
b9e29af to
361612d
Compare
The EAGLE V2 workers emit no per-request speculative spans: neither eagle_worker_v2.py nor multi_layer_eagle_worker_v2.py calls the spec_draft or spec_verify setters. The spec_stage_span scopes in eagle_worker_v2.py resolve to profile_range, which records a torch profiler or NVTX range and is a no-op otherwise, so they never reach the request trace. NGRAM opens and closes spec_draft but never spec_verify. sgl-project#40795 then removed SchedulerReqTimeStats.set_spec_verify_end_time, which had no callers left in the tree, leaving the SPEC_VERIFY stage with no code path that emits it. This restores the method, without the accepted_tokens alias that sgl-project#40795 retired on purpose. Each span is opened and closed around the operation it measures, as NGRAM already does for spec_draft: - spec_draft: around draft() in both EAGLE workers. In EAGLEWorkerV2 it sits inside the branch that drafts: the PP pre-built-verify and zero-step branches do not draft, so they do not close a draft span they never opened. - spec_verify: inside EAGLEWorkerV2.verify() and MultiLayerEagleWorkerV2.verify(), around run_eagle_verify, and around the target-verify step in NGRAM. run_eagle_verify itself is unchanged, so uno_worker_v2, which calls it directly and packs the result into its own tree output, is not affected. set_spec_verify_end_time_batch closes the span for a batch: the drafts-only count is accept_lens - 1, and accept_lens is read to the host only when tracing is enabled. Paths that go through EAGLEWorkerV2.verify() gain the verify span as well: the DP speculative prefill coordination path, and FrozenKVMTPWorkerV2, which inherits verify(). The PP tail draft is not instrumented.
361612d to
8cba278
Compare
|
@hnyls2002 — rebased onto current Two things that are your call:
Could you add |
|
@hnyls2002 — this still merges cleanly onto today's |
|
@Qiaolin-Yu — you're the other speculative-decoding Merge Oncall, and #41175 touched |
Fixes #37128.
What broke
The EAGLE V2 workers emit no per-request speculative spans: neither
eagle_worker_v2.pynormulti_layer_eagle_worker_v2.pycalls thespec_draftorspec_verifysetters. Thespec_stage_spanscopes ineagle_worker_v2.pyresolve toprofile_range, which records a torch profiler or NVTX range and is a no-op otherwise, so they never reach the request trace. NGRAM opens and closesspec_draftbut neverspec_verify.Since this PR was opened, #40795 removed
SchedulerReqTimeStats.set_spec_verify_end_time, which had no callers left in the tree. That leftRequestStage.SPEC_VERIFYwith no code path that emits it.What this does
set_spec_verify_end_time, without theaccepted_tokensalias that [misc] Remove deprecated endpoints, env vars and aliases past two releases #40795 retired on purpose. The verify span carriesnum_correct_draftsonly.set_spec_verify_end_time_batchnext toset_time_batch. It closes the span for a batch, derives the drafts-only count asaccept_lens - 1, and readsaccept_lensto the host only when tracing is enabled.spec_draft:spec_draft: arounddraft()in both EAGLE workers. InEAGLEWorkerV2it sits inside the branch that drafts: the PP pre-built-verify and zero-step branches do not draft, so they do not close a draft span they never opened.spec_verify: insideEAGLEWorkerV2.verify()andMultiLayerEagleWorkerV2.verify(), aroundrun_eagle_verify, and around the target-verify step in NGRAM.run_eagle_verifyitself is unchanged.uno_worker_v2calls it directly and packs the result into its own tree output, so a close inside the shared function would have attached a verify span to UNO as a side effect; keeping it in theverify()methods leaves UNO untouched.Paths that go through
EAGLEWorkerV2.verify()gain the verify span too: the DP speculative prefill coordination path, andFrozenKVMTPWorkerV2, which inheritsverify().Left alone deliberately
spec_draft_extend: there is noSPEC_DRAFT_EXTENDmember inRequestStageon currentmain, so restoring it would add a stage rather than fix a regression.accepted_tokensalias. [Bug] Spec V2 paths no longer emit speculative-decoding OpenTelemetry spans #37128 lists it among what NGRAM cannot emit; [misc] Remove deprecated endpoints, env vars and aliases past two releases #40795 has since retired it, and this PR follows that decision. Happy to add it back if you prefer.draft(..., with_topology=True)) is not instrumented.Testing
test/registered/unit/observability/test_spec_decode_tracing.py, CPU-only, 5 tests. They run against the realSchedulerReqTimeStatswith onlytrace_ctxreplaced — the patterntest_req_time_stats.pyuses — and assert on the emitted slices:{"num_correct_drafts": ...};accept_lensis never read to the host and no slice is emitted;set_time_batchby name exist and act.Against
req_time_stats.pyfrom currentmain, 3 of the 5 fail withAttributeError(the restored method and the batch helper); the two setter-name tests pass there, since those setters exist.Run locally on CPU: Python 3.10, the CPU CI job's
uv pip install -e "python[dev]"line with torch from the CPU index (2.13.0+cpu), andflashinfermasked as unavailable because importing it queries the GPU.test_req_time_stats.pypasses unchanged (13/13), andpre-commitpasses on the changed files.Runtime verification against a live server with tracing enabled is not included.
CI States
Latest PR Test (Base): ❌ Run #36349940952
Latest PR Test (Extra): ❌ Run #36349940591
Latest PR Test (AMD ROCm 10): ❌ Run #36349940737