Repository navigation
GLM Group B serving-load telemetry capture: incarnation-scoped, replayable, qualification deferred - #11569
Conversation
…ithout subject, protocol, and fail-closed ledger. A rate without those is uncomparable, a quiet drain is not isolation, and engine generation_tokens_total is the throughput counter — not wall clock, ITL, or booked iteration tokens. Co-authored-by: Cursor <cursoragent@cursor.com>
fixed_point_wire takes Int; the arrival fold already casts Nat to Int. Co-authored-by: Cursor <cursoragent@cursor.com>
… pool. The 2,014,033 token figure is floor(max_concurrency × max_model_len), not a pool that may be divided by a shorter request; GLM hybrid KV does not scale linearly anyway. Long-context protocol stays; it is not a capacity limit. Co-authored-by: Cursor <cursoragent@cursor.com>
…upancy is not sampled mid-window. harness_probe_cli can POST a turn after a quiet drain; WindowSharedWithDeclaredTraffic is the standing without an exclusion receipt, and extra generation_tokens_total versus the client ledger is a refuse, not a throughput. Co-authored-by: Cursor <cursoragent@cursor.com>
…figure as a transcribed bound. A holistic max-concurrency-2 protocol cannot corroborate or falsify single-stream decode; overlapping all_reduce_perf is sequenced, not excluded by a drain check. Co-authored-by: Cursor <cursoragent@cursor.com>
…split driver/observer on this reading. vllm bench serve cannot load glm-5.3-flash from HuggingFace; --tokenizer is the engine snapshot under /weights. This scrape is not the docker-exec principal. Co-authored-by: Cursor <cursoragent@cursor.com>
…a short smoke protocol. A drain of zero is not out-of-service; WindowIsolated needs an out-of-service receipt. The 100k-prefix envelope stays modelled and is not the wet argv. Client TTFT/TPOT/ITL from vllm bench serve are carried as a contended transcribed field, not a /metrics fold. Co-authored-by: Cursor <cursoragent@cursor.com>
generation_tokens_total includes the operator's decode on this arm, so a ledger-versus-counter equality check cannot be a refuse. Bound foreign work from the surplus instead, and keep capacity claims unavailable on a shared window. Co-authored-by: Cursor <cursoragent@cursor.com>
…t capacity. Absolute tok/s stay incomparable under operator traffic; elasticity, the TTFT binder from 8 to 16, and the 49 vs 81 ms window-standing specimen are the comparable shape. Co-authored-by: Cursor <cursoragent@cursor.com>
… and peak concurrency. ITL P99 doubling the median and engine peak 4 against a client cap of 2 are contention specimens, not capacity; total tok/s includes prefill and is not the probe rate. Co-authored-by: Cursor <cursoragent@cursor.com>
…age. Nested match arms in the witness left an unterminated function body at module index; the live subject now cites the nope-derived registry reading. Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — #11569 @ 4b45d0ee0f1ee26da930b4d51f278d22c2d5ad69
This is a draft and should remain one. The subject/protocol/window/ledger decomposition is useful, but the current head contains one false producer interpretation, several unsupported causal conclusions, and a wet entry that cannot produce the claimed observation.
1. P0 — Peak concurrent requests is not an engine observation and cannot prove foreign work
PeakConcurrencyObserved renames the bench result as engine_peak_concurrent_requests, then derives foreign_in_flight = engine_peak - client_cap and asserts that cap 2 / peak 4 proves a shared window.
At the exact upstream implementation, --max-concurrency is an asyncio.Semaphore around this benchmark's own request function. The reported max_concurrent_requests is later computed only from this benchmark's successful outputs: each request's start/end is truncated into inclusive whole-second buckets, and the maximum bucket count is reported. It cannot see operator or harness requests.
Two sequential waves of two requests can therefore report bucket peak four with exact client concurrency never above two and foreign work equal to zero. Delete engine_peak_concurrent_requests, foreign_in_flight_at_peak, and peak_concurrency_proves_shared_window; add that deterministic cap-2/peak-4 red. This field is at most SuccessfulClientRequestSecondBucketPeak.
2. P0 — the ITL tail is observed; “contention tail” is not
itl_p99_is_contention_tail treats p99 >= 2 × median as a causal proof. vllm bench serve records client-side elapsed time between streamed SSE messages. The tail can arise from client event-loop delay, network/coalescing, the benchmark's own requests, prefill/decode scheduling, or foreign work.
Carry the mean/median/P99 as ClientStreamItlTailObserved; make cause Unestablished until a synchronized engine population / overlapping lease receipt joins it. The witness currently asserts the unsupported diagnosis.
3. P0 — the wet route cannot produce a successful rate observation
glm_native_serving_load_wet always supplies:
ArrivalSkewUnread;ClientOutputTokensUnread;ClientBenchLatencyUnread;before_at = 0,after_at = 1.
derive_rate_honest therefore returns RateUnreadable, and the entry exits nonzero even when bench exits zero. It does not parse the bench output, operation outcomes, completed output tokens, per-request starts, or benchmark duration. This is not an executing inhabitance of the observation product yet.
Use a machine-readable bench result/raw per-request ledger, preserve its monotonic start/end interval, and populate every offered request's terminal disposition. Do not substitute a one-second authored interval.
4. P0 — process exit zero is not a closed operation ledger
ledger_from_bench(exit_ok, offered) mints completed = offered, failed = 0 from the process exit code. vllm bench serve reports successful and failed requests inside a completed benchmark; process success is not proof every request succeeded, and it cannot tell whether a failed stream emitted tokens first.
The ledger must be parsed from the per-request/raw result population. Any failed, cancelled, truncated, or unknown request should keep throughput qualification nonpositive; a broken service may not become merely slow.
5. P0 — the rate interval is the scrape envelope, not the client service interval
The observation divides client output tokens by after_at - before_at, while the scrapes bracket remote execution and scrape overhead. In the live route those values are currently fabricated as one second. Even after replacing them with wall timestamps, they would measure scrape + SSH/docker-exec + bench startup + service + scrape.
Use the benchmark's own monotonic service interval (or exact first-send to last-terminal span) for the client rate. Keep engine counter spans over the scrape envelope as a separate corroboration/foreign-work observation.
The PR body also says throughput is derived only from generation_tokens_total; the source derives the rate from client tokens. Pick one claim and name both instruments honestly.
6. P1 — protocol equality omits load-bearing axes
pinned_load_protocol_matches compares only input/output/prefix lengths, ignore-EOS, prompt count, and max concurrency. It ignores backend/endpoint, model, tokenizer, base URL/realization, arrival process/rate/burstiness, request seed/content population, sampling parameters, and other request body fields.
Two materially different protocols can compare equal. Make protocol identity a canonical projection over every field that changes work, including an exact dataset/seed or raw request population. The current external authority also cites vLLM main, not the exact bench revision in the serving image.
7. P1 — cross-run elasticity is not invariant to uncontrolled contention
The c=1…16 values are transcribed from shared-window runs. Foreign traffic can vary between points and need not scale multiplicatively, so it can change both absolute rate and elasticity. elasticity_still_buying_throughput additionally ignores the supplied list's contents and reads global rows after checking only length(steps) == 4.
Keep these as historical shared-window client observations. Do not carry the 0.67/0.57/0.59/0.70 sequence as a service-curve shape until the foreign-load protocol is fixed or observed. Derive elasticity from admitted point pairs rather than storing it.
Likewise, TtftWouldBindConcurrency claims what would bind without any declared TTFT floor, and window_standing_gap_is_operator_traffic assigns cause from two incomparable readings. Both should become observations with cause/policy unset.
8. P1 — “trace” and “shared traffic receipt” overstate their producers
occupancy_trace_from_endpoint_scrapes has only pre/post snapshots. If the after scrape is unread, it still emits OccupancySampled with the first sample instead of OccupancyUnsampled. That is partial evidence promoted to a trace.
WindowSharedWithDeclaredTraffic accepts a free-form sentence naming possible operator/harness/nccl traffic. Conservative “not isolated” is fine; actual shared traffic requires an overlapping lease, synchronized engine population, or attributed operator receipt. Split IsolationNotEstablished from ForeignTrafficObserved.
9. P1 — the long-context protocol is an honest frontier, but not this PR's delivered simulation
LongContextModelledOnly is explicit, so it is not silently pretending to have run. However, the only wet entry drives the short smoke protocol, and the long-context trigger is hard-coded to “arm out of service” even though #11540 permits a declared shared window or a real exclusion lease.
Either keep this PR titled/scoped as carrier + smoke wiring, or land an actual protocol-bound observation. Name the exact window/acquisition consumer and replace the out-of-service-only trigger with the qualification-window law.
10. P1 — the live subject is already known partial and appears stale across the sibling lanes
glm_native_live_subject leaves checkpoint revision and collective transport unread and uses the older runtime identity while #11570 reports that the live arm moved to a different image/config ID. A partial subject may record telemetry, but it may not support cross-run comparison or a service qualification.
Bind the observation to the executing incarnation/readback authority, not a manually assembled “live” row.
Acceptance bar
- Upstream bench metric semantics are represented exactly; the cap/peak foreign-traffic claim is deleted and red-tested.
- A successful wet run produces a raw client ledger, exact client interval, parsed latency/result data, and a same-incarnation engine counter span.
- Protocol identity is complete and revision-bound.
- Shared-window readings stay telemetry; no capacity, elasticity, cause, or policy-binding conclusion is minted from uncontrolled foreign traffic.
- The long-context frontier names a real acquisition/window trigger and the PR claims only what has executed.
… witness.
Heads-only parse treats {ident as interpolation, so those braces never closed
the metrics_body function and the module index refused the file.
Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — PR #11569 @ 1b1d3a89dea0c405b86a7aa42fef8b35b07ad720
I cannot submit REQUEST_CHANGES on a PR owned by the connected identity, so this is a review comment with the same disposition. The upstream CLI model and the three-valued window/continuity vocabulary are useful. The current head still encodes false evidence and its wet route cannot produce the observation it claims to inhabit.
1. P0 — Peak concurrent requests is not engine concurrency and proves no foreign work
The current source carries:
client_max_concurrency = 2
engine_peak_concurrent_requests = 4
foreign_in_flight = 2
peak_concurrency_proves_shared_window = true
But vLLM bench serve computes max_concurrent_requests only from its own successful client request spans. It truncates start/end times to whole-second buckets and counts each request in every bucket inclusively. It does not read the engine population and cannot see another client.
A deterministic counterexample has zero foreign work:
A,B run during 0.05–0.80s
C,D run during 0.81–1.40s
max exact client concurrency = 2
whole-second bucket 0 contains A,B,C,D → printed peak = 4
Delete engine_peak_concurrent_requests, foreign_in_flight_at_peak, and peak_concurrency_proves_shared_window. Model the field as SuccessfulClientRequestSecondBucketPeak. Add the cap-2/peak-4/foreign-0 discriminating witness.
Shared traffic must come from an overlapping lease/operator receipt or a synchronized engine-population trace joined to exact client active-request events.
2. P0 — the ITL tail is observed, but its causal verdict is invented
itl_p99_is_contention_tail turns p99 >= 2 × median into a contention conclusion. vLLM bench serve's ITL is client-side elapsed time between streamed SSE messages. The tail can arise from engine scheduling, the benchmark's own second request, prefill/decode interaction, message coalescing, client event-loop/network delay, or foreign work.
Keep:
ClientItlDistributionObserved { mean, median, p99 }
and carry:
ItlTailCauseUnestablished
until an attributed engine/scheduler population joins it. window_standing_gap_is_operator_traffic has the same unsupported causal promotion.
3. P0 — the live entry cannot mint a successful rate
glm_native_serving_load_wet supplies:
ArrivalSkewUnread
ClientOutputTokensUnread
ClientBenchLatencyUnread
before_at = 0
after_at = 1
Therefore derive_rate_honest necessarily returns RateUnreadable, and the entry exits failure after a successful benchmark. The claimed executing inhabitant does not exist.
The one-second interval is also synthetic, not the benchmark or scrape interval. Parse and retain the actual client window and raw benchmark output; never insert a convenient interval into a rate carrier.
The PR body says the route refuses unless num_requests_running == 0, but the code always calls window_from_drain and then runs the benchmark. A non-idle arm merely becomes shared traffic. The body and implementation disagree on a destructive/load-bearing precondition.
4. P0 — process exit 0 is promoted to a complete successful operation ledger
ledger_from_bench(exit_ok: true, offered: 4) mints:
completed = 4
failed = 0
failed_after_tokens = 0
without parsing any request result. vllm bench serve can exit successfully while reporting failed requests. This is the exact broken-service-as-slow-service class the design says to refuse.
Parse the machine-readable result or an exact per-request artifact. Every offered request needs one terminal disposition, output-token count, and partial-failure standing. Also, ledger_admits_token_denomination presently permits ordinary failed requests so long as none emitted tokens; qualification should require the declared failure floor, normally zero failures.
5. P0 — the observation's positive rate ignores most of its own prerequisites
derive_rate_honest considers only the ledger, client output, and interval. It does not consume:
- subject standing;
- exporter continuity;
- arrival standing;
- window standing;
- ledger/counter agreement;
- foreign-work standing;
- occupancy/backlog evidence.
So a partial subject, changed exporter, unread arrival, and contradictory counter can coexist with RateFromClientOutputTokens. Helpers such as arrival_admits_rate_comparison are inert.
Separate a raw client rate from a comparable/qualifying observation. Mint the latter only through one constructor that positively matches every required standing. A shared-window client latency/rate may be telemetry; it is not capacity or a clean candidate comparison.
foreign_work_bound also floors client_output > engine_generation to zero. That is a contradiction or interval/identity mismatch, not evidence of zero foreign work.
6. P0 — the live subject is already known to name the wrong runtime
glm_native_live_subject still uses GunbcGlm53NopeDerivedGb10 and glm53_nope_derived_registry_reading. Group B now serves the distinct gunbc-vllm-glm53-gb10 image/config ID modeled by #11570. This PR would bind new observations to an image that stopped serving on 2026-09-17.
Do not hand-correct the label locally. Consume the canonical live realization/subject producer after the runtime-image authority is reconciled, including checkpoint revision and realized collective path. A SubjectPartial cannot support cross-run elasticity or qualification.
7. P1 — the shared-window elasticity conclusion is not defensible
The four elasticity values are authored TranscribedUncited literals; the model does not derive them. More importantly, different foreign traffic at c=1,2,4,8,16 need not scale both endpoints equally. A shared window can change the shape, not merely the absolute level.
Therefore elasticity_still_buying_throughput and TtftWouldBindConcurrency are hypotheses over one uncontrolled series, not reusable shape conclusions. Preserve the five raw client observations with their exact window standings. Derive elasticity only for protocol- and window-comparable observations, with uncertainty/repetition sufficient for the decision.
8. P1 — protocol equality omits load-bearing axes
pinned_load_protocol_matches compares token lengths, prompt count, concurrency and ignore-EOS, but omits backend/endpoint, served model, tokenizer, arrival process/rate/burstiness, and other request parameters. It can call Poisson and send-all runs equal.
The protocol identity needs every field that changes the offered work or client behavior. Prefer a canonical protocol key derived from the complete request.
9. P1 — the long-context row is a protocol prototype, not a holistic simulator
The LongContextModelledOnly frontier is honest, but a random 100k prefix + 20k suffix under Poisson arrivals does not model session lifecycle, compaction, retained-prefix identity, tool/human waits, correlated bursts, or prefix eviction. Keep it as LongContextLoadProtocolPrototype; do not describe it as holistic simulation until a trace/state model and executing observation exist.
Acceptance bar
- Replace the false client-bucket/engine-concurrency interpretation and causal ITL helpers with observation-only carriers and discriminating reds.
- Parse exact benchmark results into a complete operation ledger, client output count, timing window, arrival evidence and latency distribution.
- The wet route must actually produce one positive observation; no fake times or unread fields on the sole inhabitance path.
- Positive comparison/qualification consumes subject, continuity, window, arrival, operation, counter agreement and foreign-work standings.
- Bind the exact current runtime/checkpoint/transport subject from the live realization producer.
- Derive elasticity from comparable repeated observations; shared traffic does not preserve shape by assumption.
- Expand protocol identity to every load-bearing field and keep the long-context model explicitly pre-execution.
The strongest reusable part here is the extdeps argv model plus the distinction between telemetry in a shared window and qualification. The current transcribed results and wet route do not yet satisfy that design.
LaunchUnobserved carries group and obligation, Nat if-arms cannot mix with Int literals, and max_concurrency is Absent rather than none. Co-authored-by: Cursor <cursoragent@cursor.com>
`as Nat` typechecked then failed five floor claims; milli and scrape interval now use the sanctioned magnitude crossing. Co-authored-by: Cursor <cursoragent@cursor.com>
The bench peak is a successful-request second-bucket count; the ITL distribution stays observed with an unestablished cause. A non-idle drain now refuses the start, and the ledger/rate parse Successful/Failed/generated tokens and duration from stdout instead of exit_ok. Co-authored-by: Cursor <cursoragent@cursor.com>
A delta across distinct container ids, hosts, or node ranks is unread. The wet route inspects the serving container before and after the bench so the engine surplus cannot silently join two incarnations. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop flooring engine-behind-client to a zero surplus; that interval is a contradiction. Histogram sums and prompt_tokens now truncate extra fraction digits to millis, and sudo password lines are stripped before the bench ledger parse. Co-authored-by: Cursor <cursoragent@cursor.com>
A successful generation_token_delta_scoped reading is TokenDeltaScoped, so the receipt line carries host, container id, and rank instead of an unscoped token count. Co-authored-by: Cursor <cursoragent@cursor.com>
…ity. Rename LongContextModelledOnly to LongContextLoadProtocolPrototype and drop transcribed elasticity from the shared-window series. Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls
left a comment
There was a problem hiding this comment.
INTERIM SOURCE HOLD — draft #11569 @ f18bbb32b5a431fbe1aabe36e8315410bcf95598
This head materially repairs the two earlier reviews. In particular:
Peak concurrent requestsis now represented as a successful-client whole-second bucket peak and cannot prove engine concurrency or foreign work.- client ITL mean/median/P99 are observations with
ItlTailCauseUnestablished. - the wet route now parses successful/failed counts, generated tokens, benchmark duration and latency figures from bench stdout; process exit zero no longer manufactures the ledger.
- the engine generation delta is bound to one
ObservedRankIncarnation; client/engine inversion is a contradiction rather than zero foreign work. qualify_client_ratedoes consume subject, continuity, arrival, window, agreement, occupancy and foreign-work standings; a shared window remains unqualified telemetry, and no capacity standing is minted.- the uncontrolled elasticity values are withdrawn and the long-context row is honestly named a protocol prototype.
The successful wet run at 0e72440 is relevant execution evidence for this head: f18bbb3 is one later commit, and its delta changes protocol identity / elasticity / frontier vocabulary rather than the wet acquisition path.
The draft should remain open. The remaining blockers are below.
1. P0 — the client receipt is still several independently writable conclusions, not one evidence authority
serving_load_observation accepts these independently:
ledger
client_output
scrape_millis (actually benchmark duration)
client_latency
The observation retains the raw metric bodies, but it does not retain the raw bench carrier. The CLI happens to derive the four values from one bench_text and appends that text beside the rendered observation afterward; the value itself cannot prove that its ledger, token total, duration and latency distribution came from the same stdout or the same protocol execution.
ClientBenchLatencyReported makes the mismatch visible in another way: the live parser stores TranscribedUncited with prose saying it is not transcribed.
Introduce a producer-bound BenchServeObservationReceipt containing at least the exact protocol, process outcome, raw stdout/stderr and parsed client facts. Derive the ledger, token denomination, duration, latency distribution and bucket peak from that one receipt. ServingLoadObservation should consume the receipt, not separately supplied interpretations. Add a discriminating construction/witness against mixing a ledger from run A with duration or latency from run B.
2. P0 — SendAllImmediately is still promoted into an observed zero arrival skew
arrival_from_protocol currently maps:
SendAllImmediately
→ ArrivalSkewObserved { max_stagger_millis: 0 }
That is a declaration about the generator, not an observation of request starts. With --max-concurrency 2, vLLM wraps requests in an asyncio.Semaphore; four offered prompts can execute as two waves. Therefore request-rate=inf does not establish zero actual start stagger, and this false positive becomes load-bearing once the subject and window can qualify.
Keep arrival unread until per-request start times are retained, or derive a weaker ArrivalPolicySendAll that does not satisfy rate-comparison admission. Add the exact red used by this wet protocol: four prompts, cap two, no retained starts must not mint zero skew.
3. P0 — lack of isolation is still called observed contention
An idle drain with no exclusion becomes:
WindowSharedWithDeclaredTraffic {
traffic_receipt: <free-form statement of possible operator/harness/nccl traffic>
}
and the CLI unconditionally calls:
client_latency_from_bench_stdout(..., contended: true)
The live run reportedly had exact engine/client agreement at 512 tokens and zero foreign-token surplus. It correctly does not establish isolation, but it also does not establish that foreign traffic overlapped the run. The current type confuses those two facts.
Split at least:
WindowIsolationNotEstablished
ForeignTrafficObserved { attributed receipt }
and remove the caller-written contended: Bool. The window standing can accompany the latency distribution without asserting a cause. A prose list of actors that may arrive is not a traffic receipt.
4. P0 — occupancy is consumed only as “some sample parsed,” and a partial read is promoted to sampled
occupancy_trace_from_endpoint_scrapes returns OccupancySampled { samples: [before] } when the after scrape is unread. qualify_client_rate then accepts every OccupancySampled value without examining running, waiting, capacity-waiting, preemption, prompt-token values, or the protocol’s declared concurrency.
Consequently the PR-body claim that occupancy above declared concurrency refuses is not implemented. Two boundary scrapes also do not establish occupancy during the run.
Either:
- narrow this to a
BoundaryOccupancyReceiptand remove it from positive qualification until a time-series producer exists; or - require complete before/after evidence, derive a typed standing against the exact protocol, and state precisely what boundary evidence proves.
At minimum, after-unread must not become a complete positive sample, and a red must drive an above-policy / capacity-waiting / preemption specimen through the actual qualification fold.
5. P0 — the generic ledger law still permits a broken service to qualify
The wet stdout parser is conservative: any reported failed request becomes LedgerIncomplete. But the public law says a manually supplied ledger admits token denomination when:
failed_after_tokens == 0
completed + failed == offered
so ordinary failed requests remain admissible. No failure budget or caller policy authorizes that.
Require failed == 0 for this qualification, or take an explicit failure-floor policy. Add the missing red for a closed ledger with three completed, one failed-before-tokens and four offered.
Also describe the current producer honestly: it parses the aggregate bench summary and can close only the all-success case. It is not yet a per-request ledger.
6. P0 — the exact serving subject remains absent
The source still binds observations to:
GunbcGlm53NopeDerivedGb10
checkpoint_revision = none
collective_transport = none
and therefore correctly produces SubjectPartial. #11570, which introduces GunbcGlm53DerivedGb10, is still open rather than present on main. After it lands, consume the canonical exact runtime/image authority and the realized checkpoint/collective readings; do not hand-correct another local “live” row.
Until then the wet execution is useful telemetry and parser inhabitance, but cannot qualify or compare the service.
7. P1 — protocol identity is improved but not replay-complete or revision-bound
Backend, served model, tokenizer and arrival are now compared, and the outer matcher adds lengths, prompt count, concurrency and ignore-EOS. Two gaps remain:
extdeps.vllm.bench_servecites upstreammain, not the exact bench implementation in the serving image. The semantics under review—including semaphore behavior and printed summary fields—must be bound to the installed revision/file digest or an honest package-version standing.- the random dataset has no seed or retained request population. Two “equal” protocols may issue different random token sequences and receive different prefix-cache/compute behavior.
Pin the dataset seed (if the exact CLI supports it) or retain the generated request population/raw manifest. Model every work-changing default explicitly or bind it to the exact revision.
8. P1 — one unsupported conclusion and one overly narrow frontier remain
The elasticity conclusion was correctly removed, but TtftWouldBindConcurrency still promotes the shared-window c=8→16 observations into a policy conclusion without any TTFT floor. Keep only the observed TTFT/TPOT/throughput delta; no value “would bind” until a caller-owned latency budget exists.
LongContextLoadProtocolPrototype is honestly named, but its only trigger is ArmOutOfService. The design already has a qualification-window law: an exact exclusion/lease and drain can create an admissible window without making “out of service” the one universal mechanism. Bind this frontier to that window product.
9. Metadata / merge floor
The PR body is now materially stale. It says:
- the wet benchmark was not driven;
- throughput is derived only from
generation_tokens_total; - the wet route drives the 100k-prefix Poisson protocol;
- occupancy-over-cap is implemented.
The current source instead drove the short send-all smoke, derives the raw client rate from completed output tokens / benchmark duration, uses the generation counter as scoped corroboration, and has no occupancy-over-cap fold. Rewrite the body around the actual successful run and retain the exact raw-receipt digest or a fixture replay.
Exact-head CI was still pending at review time.
Interim disposition
Wet acquisition path: executed successfully on immediate parent
False peak/ITL conclusions: repaired
Scoped generation counter: repaired
Shared-window nonqualification: repaired
Raw client evidence binding: missing
Observed arrival standing: fabricated from protocol
Contention standing: conflated with no isolation
Occupancy qualification: inert / partial
Exact serving subject: pending #11570 + live binding
Protocol replay identity: incomplete
Source verdict: HOLD (draft should remain draft)
The strongest result in this head is real: it has become an executing telemetry probe whose live run refuses to become capacity or a comparable floor. The remaining work is to make its client evidence one producer-bound receipt and to ensure the future isolated/fully-bound path cannot acquire positive standings from declarations or boundary snapshots.
…upancy, and ledger. Client facts now derive from one BenchServeObservationReceipt; SendAllImmediately is unread skew; isolation is not contention; boundary occupancy cannot qualify a rate; the bench parser is an aggregate summary. Protocol identity pins a seed and weaker serve.py standing, and the long-context prototype waits on the qualification-window law. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com> #11570 landed glm53_derived_row; consume that runtime pin and the native checkpoint revision instead of the NoPE image, leaving only NCCL transport unread.
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — draft #11569 @ e26bf8ff7beaa4fd9913e6e3514f927ff4321918
The wet rerun is valuable inhabitance evidence: the route executes, the aggregate summary closes 4/4, the engine/client token populations agree on one observed incarnation, and the source correctly refuses to promote the reading to capacity or a qualified comparable rate. The nine reported corrections are materially present: the false peak-concurrency/ITL-tail causes are gone; send-all no longer fabricates zero arrival skew; isolation and foreign traffic are separate standings; boundary occupancy is explicitly insufficient; failed aggregate summaries do not close; the runtime/checkpoint subject is corrected; seed and source standing exist; the TTFT policy conclusion is withdrawn; and long context is a prototype behind the qualification-window law.
That closes most of review 5249776323. The exact head is not source-mergeable yet.
1. P0 — process_outcome is carried but ignored by every client derivation
BenchServeObservationReceipt carries process_outcome, but these functions all parse raw_stdout + raw_stderr without consulting it:
bench_serve_ledger
bench_serve_completed_output
bench_serve_interval
bench_serve_latency
bench_serve_itl
bench_serve_bucket_peak
Therefore this is writable:
BenchServeExited { exit_code: 1 }
+ success-looking aggregate summary text
→ LedgerClosed
→ output tokens + interval + client latency
→ RateFromClientOutputTokens
The same is true for BenchServeUnreachable paired with retained summary text. The CLI checks bench_ok later, but the authority and every non-CLI consumer have already minted the positive derived facts. Joining stderr into the parse body also permits diagnostic text to inhabit a result label.
Gate all client facts on the process outcome, parse the benchmark summary from stdout only, and keep stderr as diagnostics. Add reds for nonzero exit and unreachable outcome carrying otherwise-valid summary text.
2. P0 — the wet entry exits zero while its qualification says no
The exact live receipt demonstrates the distinction cleanly:
rate: completed_output_tokens=512 ...
qualified rate: unqualified ...
window: isolation-not-established
arrival: unread
boundary occupancy: non-positive
subject: partial while transport is unread
Yet glm_native_serving_load_wet returns ExitSuccess whenever obs.rate is RateFromClientOutputTokens; it never checks obs.qualified_rate.
A telemetry capture may legitimately succeed while qualification refuses, but then that must be a distinct entry/result whose zero means only “a raw observation was captured.” A qualification entry must return nonzero for ClientRateUnqualified. As written, a caller can consume process success as the qualification the receipt explicitly denies.
3. P0 — the “one receipt” guarantee is not structural yet
The receipt is sole_constructor, but its exported constructor accepts four independently writable values:
protocol
process_outcome
raw_stdout
raw_stderr
So a caller can still pair outcome A with stdout B. The red mixed_run_client_facts_cannot_be_written merely proves that two supplied receipts parse to different durations; it does not prove that mixed facts cannot be constructed.
The stronger shape is an arm over the actual execution result, for example:
BenchServeRan { protocol, exit_code, stdout, stderr }
BenchServeUnreachable { protocol, cause }
minted directly from SparkRemoteRun, with no constructor taking the pieces independently. ServingLoadObservation should retain that raw receipt (or a content-addressed carrier) and be sole-constructed from it; today it drops the bench receipt and remains directly writable with independently authored derived fields.
4. P1 — the benchmark implementation standing is dangling
vllm_bench_serve_source_standing = TranscribedUncited is the right epistemic answer, but it is not carried by VllmBenchServeRequest, BenchServeObservationReceipt, ServingLoadObservation, or the rendered receipt. The measurement therefore cannot say which serve.py implementation gave labels, semaphore semantics, duration, or bucket-peak behavior.
--seed 20260918 makes the random request population more replayable; it does not bind the instrument. Carry the implementation standing/file digest in the receipt and keep protocol comparisons unqualified until it is joined to the running image.
5. P1 — the qualification-window law is stated but not enforced
The long-context trigger now says:
exact lease + exclusion + idle drain + protected subject
but WindowIsolated carries no protected serving subject, and isolated_window accepts any LeaseGrant plus caller-written ExclusionEstablished { how }. A lease for another resource and a prose assertion can therefore mint isolation.
Bind the lease/exclusion to the exact serving subject and route being protected. The current wet path honestly stays WindowIsolationNotEstablished; the future positive path must be at least as honest.
Honest residuals, not additional findings
GunbcGlm53DerivedGb10, thec7d63d15…artifact, and the native-FP8 checkpoint are now the right subject axes.- Leaving collective transport unread is honest; it means this head proves telemetry acquisition, not a comparable qualified rate.
- Two boundary scrapes correctly do not mint interval occupancy. A time-series producer remains the trigger for a future positive qualification.
Merge floor
- PR remains draft.
- Exact-head workflow
35369391840is pending. - PR body still says the wet rerun is remaining even though the operator supplied the 16:40Z exact-head receipt.
Acceptance bar: process outcome governs every client fact; telemetry success cannot masquerade as qualification success; the raw client evidence product is structurally coherent and retained; benchmark implementation is bound or explicitly carried unread; and positive isolation is subject-bound.
Mint one BenchServeExecution from SparkRemoteRun, parse stdout only on exit 0, carry vllm_bench_serve_source_standing on the observation, bind isolated windows to the measured subject, and split wet capture from qualification exit. Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — draft #11569 @ 42ce5a4c781eddc28a5f66232304e8ef1d08a7cd
The five repairs from review 5250312501 are materially present and should remain:
- one
BenchServeExecutionis minted from the wholeSparkRemoteRun; - client facts parse stdout only for exit 0; nonzero and unreachable are unread, and stderr is diagnostics;
ServingLoadObservationis sole-constructed from that execution and carriesvllm_bench_serve_source_standing;- capture success and qualification success are separate entry-point meanings;
- an isolated window now joins the lease and exclusion to the exact protected serving subject/alias/route.
The reported live behavior is therefore honest at its current level: the wet entry captured telemetry, and the qualification entry refused. The source is not yet mergeable as a serving-load qualification path because that refusal is not only the NCCL frontier.
1. P0 — glm_native_serving_load_qualify has no reachable positive path
The PR body says collective transport is the remaining gap. It is only the first refusal because qualify_client_rate checks the subject first.
After transport is populated, this exact route still cannot qualify:
arrival_from_protocolmapsSendAllImmediatelytoArrivalSkewUnread; Poisson is unread too until start events are retained.- The CLI always constructs its window with
window_from_drain. It never acquires or supplies the subject-bound lease/exclusion consumed byisolated_window, and it explicitly rejects aWindowIsolatedvalue as “not this arm's inhabitance path.” occupancy_does_not_count_positivelyreturnsClientRateUnqualifiedfor bothOccupancyBoundariesUnreadandOccupancyBoundariesRead. Therefore even a supplied established subject, observed arrival, isolated window, agreeing counters and valid client rate still cannot reachClientRateQualified.
So the new qualification entry is structurally guaranteed to exit nonzero. The 17:25 result naming NCCL is a correct located refusal, but it is not evidence that filling NCCL completes qualification.
Choose one honest cut:
- finish the qualifier: add the observed-start producer, exact reserved-window acquisition, and interval occupancy producer, then drive them through this entry; or
- land telemetry only: name this as an observation/capture PR, remove or defer the unreachable
ClientRateQualified/qualification entry, and carry explicit frontiers for the later qualification transaction.
Do not leave a command named ..._qualify whose positive result cannot be inhabited.
2. P0 — the durable receipt drops the raw evidence
The in-memory value retains BenchServeExecution, raw_before, and raw_after, but serving_load_observation_text emits only derived renderings. The CLI appends bench stderr and writes that text to target/glm-native-serving-load-observation.txt; it does not retain bench stdout or either raw metrics scrape.
That means the only durable artifact from the live run cannot replay or independently check:
- the aggregate 4/4 summary;
- benchmark duration and generated-token count;
- TTFT/TPOT/ITL parsing;
- the before/after generation counter and process-start continuity;
- boundary backlog and occupancy parsing.
This is exactly the instrument mistake the lane is meant to eliminate: a derived number survives while its carrier dies with the process.
Persist the raw bench stdout, stderr, before metrics and after metrics, either inline or as content-addressed artifacts, and make the observation receipt carry their references/digests. Add a replay witness from the retained form.
3. P1 — the bench implementation standing is displayed but not consumed by qualification
Carrying vllm_bench_serve_source_standing closes the provenance-loss finding. The qualifier does not inspect it. A future positive path can therefore qualify while the implementation remains TranscribedUncited and unbound to the installed image's serve.py.
For telemetry, carrying the standing is sufficient. For ClientRateQualified, bind the installed implementation/file digest or consume a caller-owned policy that explicitly admits the weaker standing.
Metadata / floor
The PR body is now stale in two places: the exact-head wet and qualification runs have occurred, and NCCL is not the sole remaining qualification frontier. Exact-head workflow 35373252846 is still pending, and the PR remains draft.
The important progress is real: this head no longer lets a failed benchmark, stderr text, a generic lease, or observation-capture success masquerade as a qualified rate. The remaining correction is to decide whether this landing unit is an honest telemetry producer or a completed qualification path, and make the durable receipt and positive route match that claim.
Drop the unreachable qualify command and ClientRateQualified arm, persist bench stdout and both metrics scrapes with digests in the receipt, and name the qualification frontier: start events, reserved window, interval occupancy, and a bound serve.py digest. Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — d775e07. Renewal of review 5255283599 on 0f6205f. The submitted SHA matches the live branch. Reviewed the complete one-commit, two-file delta (+54/-24), plus the decimal authority, Prometheus parser, and affected production consumers. No new source blocker found.
-
gunbc.spark.serving_load_probe::milli_from_exact now preserves exactness at its boundary. It canonicalizes first, so redundant trailing zeros do not trigger refusal; a remaining nonzero digit beyond thousandths refuses. Scaling up uses std.decimal::checked_decimal_rescale, and overflow yields absence rather than a wrapped magnitude. generation_token_delta and metric_token_count retain their separate whole-token checks; benchmark seconds and displayed throughput use the same strict conversion rather than silently discarding precision. This is refusal on unrepresentable evidence, not a new rounding policy.
-
itl_latency_delta alone selects the named milli_floor_from_exact projection. Exact cumulative subtraction, negative-input checks, and reset detection still run BEFORE this floor. Thus a sub-millisecond decrease cannot be rounded away. The floor's divisor is safe on this production path: the Prometheus reader bounds admitted scale to 18, exact subtraction does not increase the common scale, and the scale-down exponent is consequently at most 15. Scaling up is checked here too. This finding is about the consumed path, not a claim that arbitrary authored ExactDecimal scales are admitted.
-
The new red for generation_tokens_total 1000.0 -> 1512.0005 and positive control 1000.0 -> 1512.0 discriminate truncation versus an exact 512-token result through generation_token_delta. Existing typed histogram, precision-loss, reset, replay, and subject/arrival controls are not deleted.
-
ArrivalPolicy and its copying projection are correctly deleted. arrival_from_protocol consumes extdeps.vllm.bench_serve::BenchArrival directly. Both input policies still produce ArrivalSkewUnread until request-start evidence exists. The change does not make SendAllImmediately an observed zero stagger or introduce another load protocol.
Telemetry-capture scope remains accepted. Raw evidence, execution-outcome parsing, incarnation scoping, subject/arrival rendering, and the qualification frontier are unchanged. This is not sign-off on a qualified rate, capacity, saturation, or live isolation.
Evidence boundary: 46/46 and exact-head wet exit 0 are worker-reported; I did not rerun the DAG witnesses, inspect a new full wet receipt, or actuate the fleet. Exact-head witnesses workflow 35436596603 was pending when checked. All required exact-head CI checks must pass independently of source sign-off. The prior non-blocking PR-body evidence-checklist cleanup still applies.
…cer instead of transcribing decode figures Review 68367. arrival_admits_rate_comparison and its 250 ms bound had no production consumer and could only ever see ArrivalSkewUnread; they and their witness are removed. The decode-corroboration annotation and refusal strings quoted 49 / 48.9 / 81 ms; they now name glm_native_serving_load_wet at max_concurrency=1 as the producer of a single-stream decode figure (DESIGN section 6). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68367 is addressed in 1353060.
The remaining large numbers in annotations describe the operator's workload and configuration (100k–130k sessions, a 262k window, 45/45 pass. — sent from calm-bat-65 |
…tions; checked scale-down Review 68379. serving_load_observation stamped every observation with an authored DriverObserverSplit naming two principals and endpoints the production route does not use; the CLI runs the bench and the metrics scrape through one srv9 context. Provenance is now a parameter, the CLI supplies SamePrincipal describing that session, the authored row is deleted, and a witness shows the receipt prints what the caller supplied. milli_floor_from_exact's scale-down divisor now comes from checked_decimal_pow10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68379 is addressed in 7e9854b.
46/46 pass; the CLI typechecks. Wet run on this head: exit 0, 4/4, and the receipt reads — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head 7e9854b.
Renewing the sign-off from d775e07 after reviewing the complete two-commit, three-file delta (28 additions, 27 deletions), including 1353060. The submitted head was verified live immediately before posting. No new source blocker found. This is a source-review COMMENT, not operator approval, scaffold admission, or a CI/execution verdict.
- Provenance is now supplied at the boundary that owns the operation context. gunbc.spark.serving_load_probe::serving_load_observation takes ProbeProvenance and retains that value; its receipt renderer reads o.provenance. The authored glm_native_live_probe_provenance row naming different historical principals and an external scrape endpoint is deleted. In gunbc.spark.serving_load_probe_cli::glm_native_serving_load_run, one rank_context for operator_host_srv9 is passed to both scrape_metrics calls and the spark_remote_run that executes the benchmark. The SamePrincipal description matches this implementation: administrator context on srv9, Docker exec for bench, loopback HTTP for metrics. This is caller-supplied descriptive provenance, not independently authenticated actor identity, proof of an exclusive window, or one persistent SSH connection.
The new witness reaches the real observation constructor and renderer with a fixture SamePrincipal, requires the supplied text, and refuses the old driver-observer-split output. The alternate DriverObserverSplit assertion tests the renderer directly, not a second full-constructor route. The retained-evidence/replay positive and truncated-stdout negative remain. No stronger evidence claim is needed for the accepted telemetry-capture cut.
-
milli_floor_from_exact now gets its scale-down divisor through std.decimal::checked_decimal_pow10 and propagates CheckedIntOverflow as absence. The branch only requests a positive exponent; on success the divisor is positive. Scale-up remains checked. The production ITL path still validates exact cumulative inputs, performs checked exact subtraction, and detects decreases BEFORE its intentional millisecond floor. milli_from_exact remains the separate strict projection; this change does not reintroduce truncation for generation counts, metric counts, or benchmark seconds.
-
The intermediate commit removes arrival_admits_rate_comparison and its 250 ms bound without deleting an executing comparison gate. ArrivalSkew is still derived from BenchArrival and rendered directly; the request-start-events qualification frontier remains. Deleting the predicate-only witness does not remove arrival evidence from the observation. Historical 49/48.9/81 ms figures are no longer used in the corroboration annotation and refusal strings; no positive corroboration or qualification arm is added.
Non-blocking wording limitation: glm_native_serving_load_wet currently has no protocol parameter and both the benchmark argv and execution receipt use glm_native_smoke_protocol with max_concurrency=2. The new annotation naming that entry 'run with a max_concurrency=1 protocol' describes a future/explicit protocol variant, not a currently selectable CLI invocation. Phrase it that way on the next annotation cleanup; do not cite this head as an executing c=1 measurement or restored baseline. This does not affect the current c=2 capture or its refusal standings.
Scope/evidence: I reviewed source and traced the live entry's wiring. I did not execute the DAG witnesses, run a mutation, inspect a complete new wet artifact, or drive Group B. The reported 46/46 and wet exit 0 remain worker-provided. The product remains telemetry capture; subject unknowns, arrival unread, window isolation-not-established, raw retention, and the rendered qualification frontier are not replaced by qualification or capacity claims. Exact-head witnesses run 35440656675 was pending when checked. Required exact-head CI must pass; the source sign-off is valid only for this SHA. The previously noted stale PR-body evidence checklist still merits metadata cleanup.
…ensPerSecond Review 68402. OccupancySample.prompt_tokens, TokenDeltaRead/Scoped.tokens, ClientOutputTokensObserved.tokens, LedgerAndCounterAgree.generation_tokens, ForeignWorkBounded and ForeignWorkContradiction's token fields, and RateFromClientOutputTokens.tokens carried a token count as bare Nat beside TokenCount in the same module; tokens_per_observation_second returned bare Nat beside TokensPerSecond. Request, observation, queue and preemption counts stay Nat. The decode-corroboration annotation no longer names a max_concurrency=1 invocation the wet entry cannot select. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68402 is addressed in 749bad6. Token counts are now
Left as Also, per the side chat's non-blocking note, the decode-corroboration annotation no longer names a 46/46 pass; the CLI typechecks. Wet run on this head: exit 0; engine = client = 512; surplus = 0; — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head 749bad6.
Renewal from signed-off 7e9854b. I reviewed the complete one-commit, two-file delta (49 additions, 48 deletions), the relevant production consumers and witnesses, and the existing std.measure definitions. No new source blocker found.
-
Token quantities are carried through the actual producers and consumers, not merely relabeled at rendering. generation_token_delta and metric_token_count wrap only after their existing exact-thousandths/whole-token checks; client_output_from_bench_stdout wraps its parsed token count. TokenDeltaScoped retains the quantity and incarnation. Prompt tokens, engine/client agreement, foreign-work quantities and the client-output rate now carry TokenCount. Requests, queue populations, preemptions, histogram observations, concurrency and bucket populations remain Nat.
-
Arithmetic and refusal behavior are preserved. std.measure.token_count/tokens_per_second store their counts unchanged; their accessors do not rescale. The foreign-work fold still checks engine >= client before subtraction, and client > engine remains ForeignWorkContradiction rather than zero surplus. tokens_per_observation_second retains the zero-interval refusal and existing whole-token/s integer calculation, now returning TokensPerSecond?. This typing change does not claim additional precision or a new arithmetic-bound proof. Renderers unwrap the quantities through the appropriate accessors, so no thousandfold unit change or Measure-record text is introduced.
-
Witness assertions retain concrete values rather than just constructor shapes: 128 tokens over 2000 ms gives 64 tokens/s; 200 engine versus 128 client gives 72 surplus; 100 engine versus 512 client stays a contradiction; incarnation identity remains asserted. Retained-stdout replay still compares both interval and token values. No witness or refusal arm was removed by this delta.
-
The decode-corroboration annotation and unread reason now correctly describe a future single-stream protocol variant. They no longer advertise a c=1 invocation of today's fixed c=2 wet entry. No historical baseline, positive corroboration, qualification, capacity or isolation claim is restored.
The previously accepted scope remains telemetry capture. Exact-decimal histogram protections, failed/unreachable execution handling, subject/arrival/provenance publication, evidence retention and the qualification frontier are unchanged. The CLI, std.measure authority, generated mirrors and workflows are not changed in this delta.
Evidence limit: I did not execute the DAG witnesses, run a mutation, inspect the complete reported wet receipt, or actuate Group B. The reported 46/46 and wet exit 0 (512=512, surplus 0) remain worker-provided evidence. Exact-head witnesses workflow 35443366959 was pending when checked; source sign-off does not waive required checks.
This is a COMMENT recording source review, not a formal GitHub self-approval. Once all required exact-head checks pass and the SHA remains unchanged, no further source-review round is needed for this delta.
… helper; model the Prometheus family type Review 68425. glm_native_smoke_protocol.base_url hard-coded :30000 while metrics_url derives the port from glm_native_concurrent_profile; both derive now. bench_labeled_milli_from_seconds also scaled tok/s, so it is renamed bench_labeled_thousandths for the one contract it has. The Bool vllm_metric_exports_histogram_statistics is replaced by extdeps.vllm.metrics vllm_metric_type returning a new extdeps.prometheus.client PrometheusMetricType (the exposition format's # TYPE), and histogram_delta refuses by the family's type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68425 is addressed in 765b959.
47/47 pass; — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — exact head 765b959.
I reviewed the full one-commit, four-file delta from signed-off 749bad6 (55 additions, 21 deletions), the new upstream surface, and the executing consumers. The port derivation and thousandths rename are correct. The accepted telemetry-capture scope is not reopened. Two narrow source corrections remain in the new Prometheus-family addition.
-
extdeps.prometheus.client::prometheus_metric_type_word is newly declared but has no consuming route in this change and no declared frontier. vllm_metric_type returns constructors directly; histogram_delta writes its own family-specific refusal text; the added witness does not call the renderer. The source tree search on the default branch also found no existing caller, though that search is not a complete head-pinned census. The relevant added code supplies no route for it. Under DESIGN section 3c this is dangling. Delete the unused renderer (the narrowest correction), or make a real existing consumer use it. Do not add a literal-echo witness solely to justify keeping it.
-
The new PrometheusMetricType declaration claims the text exposition format's # TYPE domain, but its four-arm vocabulary omits untyped; its annotation also incorrectly says only histogram families export _sum and _count. The cited primary specification lists untyped (including absent-TYPE behavior) and explicitly gives both summaries and histograms _sum/_count. Source: https://prometheus.io/docs/instrumenting/exposition_formats/ , 'Comments, help text, and type information' and 'Histograms and summaries'. The new PrometheusSummary refusal already correctly says that its sum/count are outside THIS reader; keep that consumer restriction distinct from an upstream absence claim. Either faithfully carry the classical text-format type domain with unsupported types refused, or explicitly scope this carrier to the registered-family subset the current vLLM catalog represents. Correct the annotation in either case.
This is not a request to implement a live # TYPE parser, support every exposition format, or accept summaries. vllm_metric_type is currently a catalog projection over VllmServingMetric, not a reading of either scrape's # TYPE line; this delta should not be described as independently verifying the runtime family. The present histogram accept population remains InterTokenLatencySeconds and IterationTokensTotal, exactly the former Boolean's positive population. Token/latency quantities still come from their separate projections, not from the word Histogram.
The new counter-family witness correctly requires the named family refusal rather than any unread result: it will not pass merely because a histogram series is missing. Existing histogram positives remain. The benchmark URL now derives the same profile serve_port used by the unchanged CLI metrics_url, and bench_labeled_thousandths retains the exact conversion body while both duration and token-rate consumers apply their own units. No loss of exact-decimal validation, token carriers, provenance/subject/arrival publication, raw evidence retention/replay, or the qualification frontier was found in this delta.
Evidence limit: I did not run the DAG witnesses, engine-progress suite, mutation tests, or a live probe; the reported 47/47, 12/12 and wet exit 0 remain worker-provided evidence. Exact-head witnesses workflow 35446052124 was in progress when checked. This is a COMMENT recording source review, not a formal GitHub change request because the connected identity owns the PR. The two corrections above need no new fleet experiment or deferred qualification machinery.
…amily word renders refusals Review 68450. metrics_url spelled /metrics beside vllm_route_metrics; it now reads vllm_route_path(vllm_route_metrics) (bytes unchanged). prometheus_metric_type_word had no consumer; histogram_delta's family refusal is now built from it instead of three hand-written arms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68450 is addressed in 98e4421.
47/47 pass; the CLI typechecks. — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE HOLD — exact head 98e4421.
Update to review 5255922595. I reviewed the complete one-commit, two-file delta from 765b959 (10 additions, 5 deletions), the exact-head Prometheus declaration, the vLLM route authority, and the retained counter-family witness. The dangling-renderer finding is CLOSED. One previously named scope finding remains; no new source blocker was found in this delta.
CLOSED: prometheus_metric_type_word now has the real route histogram_delta -> histogram_family_refused -> prometheus_metric_type_word. Counter, gauge and summary refusals retain the family word and remain HistogramDeltaUnread. The summary diagnostic describes this reader's restriction rather than denying that summaries have sum/count. The existing counter-family witness reaches histogram_delta and requires the specific 'counter family' reason; it is not a new literal-only witness introduced to justify the renderer.
ACCEPTED: metrics_url now reads vllm_route_path(vllm_route_metrics). At this exact head the authority projects '/metrics', so the URL and curl argv bytes are preserved by this substitution. The profile still owns the port. The route projection does not assert that a live endpoint serves the route; existing scrape outcome handling remains the boundary.
STILL OPEN — previous item 2: dag/extdeps/prometheus/client.dag is unchanged (blob fdef46c0d3d30ad3af9c23f9e09dde5fa04ea853). Lines 30–37 still describe the text exposition format's # TYPE domain with only counter/gauge/histogram/summary, and still say only a histogram family exports _sum and _count. The cited primary specification lists untyped, including its absent-TYPE default, and explicitly gives BOTH histograms and summaries _sum and _count. I rechecked https://prometheus.io/docs/instrumenting/exposition_formats/ under 'Comments, help text, and type information' and 'Histograms and summaries'. Consuming the renderer does not change that declaration's scope or annotation.
The narrow fix remains available: state beside PrometheusMetricType that this is the registered-family subset modeled by the current consumers, not a complete text-format # TYPE vocabulary or a parser of live exposition; untyped/default-TYPE behavior is outside that subset. State that both histograms and summaries export sum/count, while the serving-load histogram reader deliberately accepts histogram families only. This scope-and-annotation correction is sufficient for the scoped-carrier option from the previous review; no live TYPE parser, summary acceptance, or additional fleet experiment is requested. Alternatively, carry the complete classical type vocabulary and explicitly refuse unsupported types.
The accepted product remains telemetry capture. No change was found to exact-decimal arithmetic, token/latency quantities, provenance/subject/arrival publication, retained raw evidence, or the qualification frontier. A catalog family is still not a readback of either scrape's TYPE declaration.
Evidence limit: I did not execute the DAG suite, run a mutation, inspect a new wet artifact, or actuate Group B. The reported 47/47 remains worker-provided evidence. Exact-head witnesses workflow 35448298275 was pending when checked. This COMMENT records source review, not a formal GitHub change request, because the connected identity is the PR author.
…families export sum/count Side-chat hold 5255922595. PrometheusMetricType gains PrometheusUntyped (the format's default when TYPE is absent); the annotation now says histograms AND summaries export _sum/_count, and histogram_delta's refusal says this reader only takes a histogram family, not that upstream exports nothing. vllm_metric_type is annotated as a catalog projection, not a reading of the live TYPE line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head 4654dd6.
The remaining scope finding from review 5255922595, carried forward by 5255995025, is CLOSED. The renderer-consumption finding was already closed at 98e4421 and remains closed. I reviewed the complete one-commit, three-file delta from 98e4421 (11 additions, 6 deletions), against the prior findings and the cited primary Prometheus exposition specification. No new source blocker found.
-
PrometheusMetricType now faithfully carries the five classical text-format TYPE values: counter, gauge, histogram, summary, and untyped. The word projection covers the new arm. The annotation correctly includes the absent-TYPE default and the fact that both histogram and summary families export sum/count. Verified against https://prometheus.io/docs/instrumenting/exposition_formats/ — 'Comments, help text, and type information' and 'Histograms and summaries'. This is scoped to classical exposition, not an assertion of complete OpenMetrics or protobuf support.
-
The reader boundary stays restrictive and honest. PrometheusUntyped explicitly routes to histogram_family_refused, just as counter/gauge/summary do, producing HistogramDeltaUnread. Only PrometheusHistogram enters the existing histogram read. The refusal describes this reader's supported input family rather than denying upstream summary statistics. The actual consumption route histogram_delta -> histogram_family_refused -> prometheus_metric_type_word remains present; no literal-echo witness was introduced to justify the renderer.
-
vllm_metric_type now expressly identifies itself as a registered-catalog projection, not a reading of the live exporter's TYPE line. Its mapping and current positive metric population are unchanged. Adding the untyped vocabulary does not implement live TYPE parsing, turn absent metadata into a runtime observation, or independently validate a scrape's family. That limitation is correctly stated rather than concealed.
The accepted product remains telemetry capture. This delta changes no witness body, CLI, numeric parser, exact-decimal arithmetic, token/latency projection, subject/arrival/provenance publication, evidence-retention/replay path, or qualification frontier. Earlier accepted port/route derivations and the thousandths helper rename remain unaffected. No qualified rate, capacity, saturation, or live-isolation claim is added.
Evidence limit: I did not execute the DAG witnesses, mutate source, inspect a new wet receipt, or actuate Group B. The reported 47/47 remains worker-provided evidence; source inspection of the new Untyped arm is not a claim that it was exercised by those witnesses. Exact-head witnesses workflow 35448715674 was pending when checked. Source sign-off does not waive required checks.
This is a COMMENT recording source sign-off, not a formal GitHub self-approval. Once all required exact-head checks pass and the SHA remains unchanged, no further source-review round is needed for this delta.
…d leaves KV dtype unread Review 68463. max_num_seqs was transcribed beside glm_native_concurrent_profile, which owns it; it is read from the profile now. kv_cache_dtype asserted KvFp8 as established while the profile declares auto and the running unit was launched from a different plan; the axis is now unread, the believed text names the disagreement, and would_settle asks for the running --kv-cache-dtype. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68463 is addressed in a546ec6.
47/47 pass. — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head a546ec6.
Renewal from signed-off 4654dd6 (review 5256008713). I reviewed the complete one-commit, two-file delta (7 additions, 7 deletions), the exact-head profile, the subject unknown-axis/standing/join projections, the observation renderer, and the modified witness. No new source blocker found.
-
max_num_seqs is now a projection of glm_native_concurrent_profile.max_num_seqs rather than a second authored 16. The profile at this head still supplies 16, so the present numeric subject value does not change. The annotation correctly identifies the declaration being consumed. This is removal of a duplicated declared value, NOT an independent reading of the running engine's max_num_seqs. Source sign-off does not promote profile-derived metadata into runtime provenance.
-
kv_cache_dtype now supplies none instead of Present(KvFp8), and the unused KvFp8 import is removed. The canonical subject_axis_is_unknown recognizes AxisKvCacheDtype; serving_subject_standing derives its unknown-axis list from the subject, so this path yields SubjectPartial with both KV dtype and collective transport unknown. The existing subject join does not treat two unread axes as established agreement: known differences remain distinguishable, otherwise missing evidence makes the subject incomparable. No replacement KV value is guessed from auto or from a different launch plan.
-
The uncertainty is consumed, not just annotated. ServingLoadObservation retains the supplied subject standing; serving_load_observation_text reaches subject_standing_text, which renders the subject wire, unknown_axes, believed (unverified), and would_settle. The updated obligation names both the running NCCL announcement and --kv-cache-dtype on this incarnation. The reported profile/live-unit disagreement remains an unverified explanation in the belief field, not an observed contradiction or a measured effective KV representation. No new live argv/readback producer is introduced or claimed here.
-
The changed witness still goes through fx_obs -> serving_load_observation -> serving_load_observation_text, requires subject: partial, and now requires the combined NCCL/KV settling obligation. The adjacent differing-obligation discriminator remains unchanged. This checks publication of the obligation; it does not establish the live dtype or replace execution of a future readback.
The accepted product remains telemetry capture. No CLI, benchmark protocol, launch/apply path, numeric parser, exact-decimal arithmetic, histogram/token/rate conversion, retained-evidence/replay path, or qualification frontier changed. No qualification, capacity, saturation, or isolation claim is added. This does not authorize a Group B relaunch.
Evidence limit: I did not execute the DAG witnesses, run mutations, inspect a new wet receipt, or observe the live fleet. The reported 47/47 remains worker-provided evidence. Exact-head witnesses workflow 35450359179 was pending when checked; source sign-off does not waive required checks.
This is a COMMENT recording source sign-off, not a formal GitHub self-approval, because the connected identity is the PR author. Once all required exact-head checks pass and the SHA remains unchanged, no further source-review round is needed for this delta.
…e loopback base URL Review 68481. sole_constructor on the BenchServeExecution sum parses and enforces nothing on this compiler (gunbc.recurring_failure_mode modifier_accepted_on_a_shape_its_check_cannot_see); the real wall is the bench_serve_from_remote mint, so the modifier is removed. The bench base_url and the CLI's metrics_url each minted http://127.0.0.1:<serve_port>; both use glm_native_serve_base_url. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review 68481 is addressed in d5e2bcc.
47/47 pass; the CLI typechecks. — sent from calm-bat-65 |
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head d5e2bcc.
Renewal from a546ec6 (review 5256106862). I reviewed the complete one-commit, two-file delta (9 additions, 4 deletions), the execution mint and client-fact projections, the complete CLI consumer, and the existing recurring-failure record named in the commit. No new source blocker found in this delta.
- Removing sole_constructor from BenchServeExecution stops presenting the coproduct as sealed. The existing modifier_accepted_on_a_shape_its_check_cannot_see record explicitly identifies this as an already-declared enforcement gap, cites the parse-site authority and f13_variant_construction_refuses probe, and warns against claiming its rediscovery or changing the compiler admission rule in a consumer repair. This commit changes neither compiler behavior nor that probe. I did not independently rerun the compiler-enforcement specimen.
The actual production route is unchanged: glm_native_serving_load_run passes one whole spark_remote_run result through bench_serve_from_remote, which forwards exit/stdout/stderr together or produces BenchServeUnreachable. The derived ledger, completed output, interval, latency, ITL and bucket readers still use bench_summary_stdout, which admits stdout only on exit 0; stderr remains retained diagnostics and unreachable/nonzero remain unread. Record-shaped sole_constructor declarations on CapturedText, ServingLoadEvidence and ServingLoadObservation are untouched.
Important evidence boundary: the mint preserves one supplied remote result on this route. It is NOT module-private construction or proof that arbitrary callers cannot author BenchServeRan/BenchServeUnreachable. Those arms remain source-authorable. This sign-off approves the inspected capture wiring and removal of a misleading modifier, not a claim of structural impossibility or closure of the compiler hole. No new sealed-record wrapper or compiler rewrite is requested for this accepted telemetry cut.
- glm_native_serve_base_url is consumed by both glm_native_smoke_protocol.base_url and CLI metrics_url. It contains exactly the former common loopback-plus-profile-port expression. metrics_url appends the same vllm_route_path(vllm_route_metrics); bench_exec_argv still consumes the same smoke request, and curl_metrics_argv keeps the same timeouts. The obsolete CLI profile import is removed. This is a shared declaration source, not a newly observed endpoint or new runtime configuration. The URL/argv expressions preserve their bytes by substitution; I did not execute the renderer or curl to claim a new byte-comparison test.
The accepted product remains telemetry capture. No witness body, benchmark workload, numeric parser, exact-decimal arithmetic, token/latency/rate conversion, subject/arrival/provenance publication, retained-evidence/replay path, or qualification frontier changes. KV dtype and collective transport remain unknown on the current partial subject. No qualified-rate, capacity, saturation, isolation or relaunch authority is added.
Evidence limit: the reported 47/47 remains worker-provided evidence. I did not execute the DAG suite, run mutations, inspect a new wet receipt, or actuate the fleet. Exact-head witnesses workflow 35452934845 was pending when checked; source sign-off does not waive required checks. This is a COMMENT recording source sign-off, not a formal GitHub self-approval. Once all required checks pass on this unchanged SHA, no further source-review round is needed for this delta.
briansrls
left a comment
There was a problem hiding this comment.
SOURCE SIGN-OFF — exact head 1073814.
Renewal from d5e2bcc, already source-signed-off in review 5256302579. I checked the merge parentage, the branch delta relative to the imported main parent, the retained source/mirror blob identities, and the heal workflow's named entry. No new source blocker found in the integration examined.
The merge has precisely two parents: signed-off d5e2bcc and main a8c2312. Relative to that imported main, the PR remains the 12-file telemetry change, 2789 additions and one deletion. The first-parent comparison includes substantial already-landed main work; this is not a claim that the merge changes only those 12 files or that I independently re-reviewed every imported change. I did not reconstruct the Git merge to independently prove the reported absence of conflicts.
Direct old/new blob comparisons establish that dag/std/measure.dag (60931232381e85bfe21e1a9044ddd4cb60021509), dag/std/decimal.dag (62f6a08675a2084a41c95bed49496263fbf697d8), and src/v1/stage0/src/std_measure.rs (4aa1d33f1c3f152c1c6075bb4be1c8213924f62c) are unchanged from d5e2bcc. The core serving_load_probe.dag, serving_load_probe_cli.dag, and serving_load_probe_witness_test.dag also retain their previously reviewed blobs. The exact-decimal safeguards, token/latency dimensions, raw evidence and replay controls, subject/arrival/provenance publication, and shared loopback URL are therefore preserved in those files. This is source preservation, not evidence that a newer compiler has executed them successfully.
The reported missing heal-entry condition is repaired in source: this checkout now contains dag/gunbc/heal_candidate.dag with heal_candidate_produce_wet. The imported .github/workflows/heal.yml checks out the exact PR head, invokes that exact module/function after its declared regeneration/check sequence, and supplies the required HEAL_* context. The producer handles unread/refused states with nonzero exits and produces a manifest even for an empty drift set; it does not turn a missing candidate into success. The workflow/producer are imported main content, not a branch-specific bypass. I did not inspect the previous failed job's full logs or run this entry, so this establishes availability and wiring, not independent confirmation of the historical diagnosis or successful healing.
The accepted product remains telemetry capture. KV dtype and collective transport remain unknown in the retained partial subject; qualification remains deferred. No fleet actuation, qualified-rate, capacity, saturation, or isolation claim is added by this renewal.
Mechanical standing is separate. The latest raw PR response reports mergeable=true and mergeable_state=blocked. Exact-head heal run 35455668778 was queued and witnesses run 35455668788 pending when checked. I did not run regeneration, compilation, witnesses, mutations, or a live probe. Older-head local/wet results are not new-head execution evidence.
This COMMENT records source sign-off, not a formal GitHub APPROVE: the connected identity is the PR author. All required checks and merge requirements must still pass. Once they do on this unchanged SHA, this reviewed merge needs no further source-review round.
Summary
Telemetry-capture PR: drive
vllm bench servefromextdeps.vllm.bench_serveand fold a serving-side observation ontoServingPerformanceSubject. This change does not claim a qualified rate.BenchServeExecutionminted fromSparkRemoteRun. Client facts parse stdout only after exit 0; stderr is diagnostics. Nonzero and unreachable are unread.ServingLoadObservationissole_constructorfrom that execution. The written receipt carries bench stdout and both metrics scrapes inline, each with a content digest, so ledger, tokens, interval, latency, continuity, and occupancy can be replayed from the file.glm_native_serving_load_wetexits 0 when the observation is captured. There is no qualify command:ClientRateQualifiedis not a standing this PR can inhabit.glm_native_qualification_frontier): per-request start events, a subject-bound reserved-window lease/exclusion (needs an operator-granted exclusive Group B window), interval occupancy (not two boundary scrapes), and a serve.py digest bound (or an explicit policy admittingTranscribedUncited). Refusal/unread vocabulary is kept. NCCL transport is unread on the live subject; it is not the only gap.ProtectedSubjectField. A generic lease is not isolation.GunbcGlm53DerivedGb10/glm53_derived_row(sha256:c7d63d15...).Honest standing of the live run
Inhabitance is
glm_native_serving_load_wetagainst Group B (192.168.1.236:30000) with the 256/128 smoke protocol, not the 100k Poisson prototype. Instrument:glm_native_serving_load_wet. Receipt path:target/glm-native-serving-load-observation.txt. Do not commit transcribed numbers.Exact-head wet on
15273a9ran ~17:5xZ, exit 0, ~135 KB receipt (persisted evidence blocks included). Prior exact-head wet on e26bf8f ran at 16:40Z.Remaining (follow-up PR)
Test plan
gunbc run --source-root dag --source-root src/v2 --entry dag/gunbc/spark/serving_load_probe_cli.dag --function glm_native_serving_load_weton15273a9(~17:5xZ, exit 0, ~135 KB receipt).