Skip to content

api: logprobs, top_logprobs and echo on the OpenAI endpoints, aligned by raw-stream span - #1353

Open
monotophic wants to merge 6 commits into
JustVugg:devfrom
monotophic:api/server-surfaces
Open

monotophic wants to merge 6 commits into
JustVugg:devfrom
monotophic:api/server-surfaces

Conversation

@monotophic

@monotophic monotophic commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic

This is #1353 reduced to its logprobs work, rebuilt on current dev as three commits; the seed change (#1720) and the checked engine writes (#1721) have since merged and are taken in here; the array prompt and a full-stream field are separate proposals. Your items land here as the table says; 7 and 8e are in the array-prompt and engine-write PRs.

What it does. /v1/completions takes the legacy integer logprobs (0–32) with echo;
/v1/chat/completions takes boolean logprobs with top_logprobs. The server requests the
engine's existing logprobs=k channel only from a glm engine and refuses the fields with a named
400 elsewhere. Each record is located in the returned text by its span in the raw engine stream:
the stop filter, the thinking split and the tool-call parse each report the spans they emitted, and
the spans compose, so "".join(tokens) == text and "".join(logprobs.content[*].token) == message.content hold exactly, including for a token a stop cut in half. A request that never asks
for logprobs writes the byte-identical SUBMIT header; two tests pin that against dev. A logprobs
request normally forfeits prefix-cache reuse, because the channel re-prefills from position 0.

item what changed the test that fails without it
1 a leading marker swallowed by the stop filter no longer drops every record; your three-record shape keeps two of three, because the marker never reached the client test_swallowed_leading_marker_keeps_the_other_two_records and its completions twin test_swallowed_leading_marker_offsets_index_the_returned_text
2 there is no accept timeout in this tree; a logprobs request waits as any request waits, so nothing releases an admission before the engine's turn ends. Engine.generate() gains one keyword, gbytes_before_ext=False, and every existing caller's wire is unchanged the two SUBMIT pins; a bounded wait is offered separately
3 a record's bytes stay its frame's own payload; a stop cut mid-token reports the characters that reached the client your echo:true, logprobs:1, stop:["lo"] shape: test_echo_keeps_the_prompt_and_the_emitted_prefix (text promptHel, offsets [0, 2, 6]); bytes: test_a_truncated_entrys_bytes_do_not_describe_the_withheld_text
4 taken the way you preferred: span reporting at each stage, alignment by lookup the three LogprobsStageCompositionTest methods that assert each surviving entry's own logprob value
5 echo: null and false normalise to absent on both endpoints test_echo_null_normalizes_to_absent_both_endpoints, test_zero_false_null_normalize_to_absent, test_chat_echo_false_normalises_to_absent_over_http
6 the environment variable you saw was in the branch you reviewed; it is not in this tree —
8a a malformed ECHO position fails that one request with its own code, engine_echo_position_malformed; the dispatcher keeps serving test_a_malformed_echo_position_spares_a_request_that_is_in_flight
8b one stop/trim/echo tail for both completion shapes its call sites; batch members call it in the array-prompt PR
8c two capability flags; the refusal names the gate it checked, not the engine's arch; the second flag is first consumed by the array-prompt PR the three CapabilitySplitIndependenceTest methods and test_the_capability_refusal_names_the_gate_not_the_engines_arch
8f the tuple rebuild and the unreachable filter were in the branch you reviewed; neither is in this tree —
8g the arrays describe exactly the returned text; reasoning and tool-call tokens are reported nowhere and the docs say so; a field for the full stream is proposed separately test_content_is_a_projection_of_the_stream_not_a_subset, which asserts the entry count before what is absent
9 on origin/dev directly, no merge commits, no trailers git merge-base HEAD origin/dev = origin/dev; trailer count 0

Changed by design against dev. These, and only these, answer a named 400 where dev answered
200: a non-boolean echo on either endpoint; echo: true on chat; echo: true without logprobs
on completions; an invalid top_logprobs while logprobs is off; and a logprobs value that is
falsy but neither false nor null (0 on chat; '', [], {} on either endpoint), which dev
ignored by truthiness. logprobs: 0 on completions still answers 200, and a valid top_logprobs with
logprobs off stays a no-op. The 400→200 shapes are the feature: logprobs 1–32 on completions and
logprobs: true on chat.

Engine faults are named, not silent. A malformed numeric tail, a malformed ECHO position, a duplicate top-k
label, records that stop short of the generated stream, and a pinned prefix that was not echoed each answer with
their own code and param. A faulted turn writes one STOP, waits for the engine's own terminal frame with its
admission held, and only then answers; a client cancellation still wins on the two terminal frames that report
nothing else, an ERROR CANCELLED or a DONE; if the STOP itself cannot be written, the request answers the
engine-write error, the larger fact. /v1/brio's header and behaviour are unchanged.

Tests. 129 test methods added in c/tests/test_openai_server.py, run from c/; 149 of the head's
tests fail against dev's server module, all in the classes this PR adds and none in an existing class,
including none of this week's assistant-continuation classes. test_openai_server.py, test_brio_api.py
and test_systemone_api.py: 360 passed, 153 subtests. Module wall time 24.7 → 26.1 s, no new test over
0.30 s. make check: 1512 tests, OK.

On hardware. From a stacked build of this work on one glm engine (spark2, the glm52_i3g64_probe
container), not this PR's commit, which differs from it in the server module, and with the battery cell
otherwise incomplete: a census of six echo-coverage shapes at N=42 produced no coverage refusal and every
served response carried its records; the fourteen requests carrying a stop each had it fire; the join identity
held on all 11 item-1 shapes (one generated no text) and on 3 item-8g thinking turns; the item-3 shape returned
a matching echoed prefix on all 6 reps, though stop: "lo" never fired on this model and a stop that did was
substituted; echo: null was served a 200 on both endpoints; k=32 returned 32 candidates per position with no
duplicate; the plain path's SUBMIT header was byte-identical to the base binary's.

Edo771977 added a commit to Edo771977/colibri that referenced this pull request Sep 14, 2026
Integrazione PR upstream: CUDA Qwen3.8 (JustVugg#1424), QLoRA (JustVugg#626), API logprobs/echo (JustVugg#1353)
@monotophic

Copy link
Copy Markdown
Contributor Author

Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic

Rebased onto current dev (87c54a93). No functional change on this branch's side — the 11-commit chain is preserved unsquashed, same messages, same content.

One file needed conflict resolution, c/tests/test_openai_server.py; all three hunks are the same shape (dev and this branch each added things at the same two places, no overlap):

  • The stdlib import block and the from openai_server import (...) list: dev's COLI_IMAGE_ROOT work added import os and _image_bytes_from_url; this branch adds import queue, LOGPROBS_TOP_K_CAP and logprobs_options. Resolved as the union.
  • The end of the file: dev added ImageUrlPathGuard and ContextExceededMessageTest ([Feature]: #1376); this branch appends its logprobs/echo test classes at the same point. Resolved as the union, dev's classes first.

c/openai_server.py merged with no conflict; its changed lines are identical against the new base (only hunk positions moved). Read-through of the merged file for each surface this branch touches: dev's deepseek-v41 image-expansion arm in chat_completion still falls through to generation(), so the logprobs/echo capability gate applies to it like every other family; dev's GLM-5.3 think-block change (#1278) lives in starts_in_reasoning, orthogonal to this branch's deferred Anthropic 200.

Re-verified on 2edcea62: make check exit 0 (1155 tests); tests.test_openai_server and tests.test_anthropic_messages (378 tests) green; lint on changed lines: no findings.

@JustVugg: Checking in on the viability of this PR. Is there interest in merging this support for deeper instrumentation through the OpenAI API interface for Colibri? See my comments on PR #1102 for more context.

I've got everything I need integrated and working at my end so my container experiments are not blocked by this stuff sitting unmerged, but I would prefer not to be investing in an isolated fork and rebasing regularly to avoid drifting too far from what you are doing here. Steering input is welcome, my hope is that these contributions would be generally useful for anyone wanting to use Colibri for serious work, including model tuning/development and research.

@JustVugg

Copy link
Copy Markdown
Owner

Reviewed. This is the strongest of your four and the only one that is genuinely independent: a different branch, no file overlap with the evidence stack, no engine source, and the engine you build from it is identical to the one built from dev because the diff is Python only.

It also unblocks something concrete. The engine on dev already implements the numeric channel, logprob_tail() in c/colibri.c and the logprobs=k/ids=1 parsing in c/decode_batch.h; only the sender was missing. And the array-prompt support is what lets lm-eval send tokenised loglikelihood requests at our own models, which dev rejects outright today. The _write_all short-write loop on the raw stdin pipe is a real latent fix on its own: an unchecked short write desyncs engine framing.

One question before I merge, and it is the only default-path behaviour change in the diff. The /v1/messages streaming commit point moves from scheduler admission to the engine's ACCEPT. On an in-tree engine that is one round trip and the event order is unchanged, but against an engine old enough never to send ACCEPT, a cold multi-minute prefill now sends zero bytes where dev sent message_start plus pings, and a client with a short idle timeout would newly time out. You name this yourself and say it has no test for that pairing.

Would you split that single commit out? The rest lands immediately and the commit point can come back with its own test.

One thing to flag rather than block on: the server suite goes from 161 to 336 methods, about 96 seconds of a 140-second Python suite. That is a permanent tax on everyone's make check. Worth it here, because an HTTP contract with error codes and batch semantics is exactly the shape table-driven tests suit, but worth knowing.

@monotophic

Copy link
Copy Markdown
Contributor Author

Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic

@JustVugg Thank you! Revised as requested. Pushed a2c42bc3, rebased onto current dev (cc756a63, no conflicts).

  • The commit that moved the /v1/messages streaming commit point to the engine's ACCEPT is out, along with the one test that pinned it. The Anthropic streaming handler is byte-identical to dev's again. It will come back as its own PR with the test you named: an engine that never sends ACCEPT behind a cold prefill, and what the client sees meanwhile. The other ten commits are unchanged (range-diff: all =).
  • On the suite cost — you were right to flag it, and it was worse than a tax: three fixture defects, not real work. Six of the new per-test server helpers registered their cleanups in the order scheduler close, shutdown, server close, thread join; unittest runs cleanups last-in-first-out, so the join ran first against a still-serving thread and waited its full 2 s, then shutdown paid the 0.5 s poll. Two engine-write tests left a fake pipe open so Engine.close() joined its reader for the full 5 s. And every fixture started serve_forever at the 0.5 s default poll interval, so each shutdown blocked half a second. One added commit fixes all three; no assertion changes, openai_server.py untouched. tests.test_openai_server on this machine: dev 22 s → previous head 96 s → now ≈9 s, with 346 methods.
  • PR body updated to match: the commit-point material removed, capstone anchors re-resolved at this head, and the cost paragraph corrected.

@JustVugg JustVugg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this. I want to say up front that the shape is right: this is U7b landing against a U7a wire side that already works, the capability gate is a single flag set once from the arch, the logprobs/top_logprobs null handling is careful, and 174 tests against 1122 lines of server code is the ratio I wish every PR had. I read c/colibri.c and c/decode_batch.h alongside the diff and the server's assumptions about the SUBMIT extension arm and the ECHO field layout check out, including the 7th gbytes field needing to precede the first key=value token, which gbytes_field correctly emits as " 0".

It is not mergeable yet. Below are the things I need fixed, worst first. I reproduced the ones marked "reproduced".

1. The default glm chat path returns empty logprobs, silently

_trim_generated_records_to_text drops every generated record when the first record's text is not a prefix of the returned text. That is the default, not an edge case: with no client stop, stop_policy returns ignore_leading=True, StopFilter swallows a leading role marker, and the prefix check fails on record 0, so the loop breaks having kept none.

Reproduced: records [b'<|user|>', b'Hello', b' world'], raw_text == 'Hello world', result is 0 of 3 kept. The client gets a 200 with a full message.content and logprobs: {"content": [], "refusal": null}. No error, no signal.

The docstring lists the thinking-split and tool-call cases as unhandled. The ignore-leading case is a third one and it is the path most requests take.

2. The ACCEPT timeout violates the invariant documented twelve lines below it

The new 503 is raised from the queue.Empty branch of Engine.generate()'s event loop. That abandons an in-flight engine turn and releases the scheduler admission, and no CANCEL is ever written. The comment immediately below says exactly why not to:

Do NOT raise here: this thread holds the scheduler admission, and releasing it before the engine confirms the cancel lets the next request SUBMIT into a pipe the busy engine is not reading, every later request then hangs silently behind the orphaned generation.

Concretely, with kv_slots > 1: request A is in a multi-minute cold prefill so the mux loop is not reading stdin. Request B opts into logprobs, its SUBMIT is written but unread, no ACCEPT inside 30 s, B raises 503 and frees its admission. The engine later generates B to max_tokens with nobody reading, and request C inherits B's cache_slot and gets SLOT_BUSY, then a 500.

docs/api.md says the request is "treated as cancelled". It is not. Either send the CANCEL and wait for confirmation before returning, or handle the missing ACCEPT somewhere that is allowed to fail.

3. echo: true deletes text the client already earned

With a stop sequence matching mid-token, text is rebuilt from the trimmed logprobs tokens, so the emitted prefix disappears from the response body.

Reproduced: /v1/completions with echo: true, logprobs: 1, stop: ["lo"], one DATA frame decoding to "Hello". StopFilter emits "Hel" and matches. The trim checks "Hel".startswith("Hello"), keeps 0 of 2 records, and text = "".join(logprobs_obj["tokens"]) overwrites the body with the prompt echo alone. Without echo, the same trim leaves tokens/token_logprobs/text_offset empty while text is "Hel", so the arrays no longer describe the returned text either.

4. Root cause behind 1 and 3

Realigning records to the returned text by prefix-matching decoded bytes is a symptom patch, and each further text transformation (thinking split, tool-call parse, inkling split) will need another special case on the same walk. The general fix is one level down: have StopFilter and the splits report how many characters of the raw stream they consumed, or have Engine.generate() tag each record with its raw-stream character span, so alignment becomes a lookup instead of a guess that silently yields an empty array when it misses.

I would rather see that than three more special cases, but I will take correct special cases if you prefer to keep the change contained.

5. echo: null is a new 400 for clients that never asked for echo

Verified against the real function: logprobs_options({'echo': None}, chat=True, ...) and the chat=False form both raise APIError(400, "echo must be a boolean."). Any SDK that serializes its full request model with nulls now gets a 400 on every chat completion and every plain completion, with no logprobs involvement at all. On /v1/chat/completions echo was previously ignored entirely, so this is a new rejection of a field chat clients never meant to set.

The rule is already in your own docstring: "top_logprobs: null is normalized to absent, same as logprobs: null". Apply it to echo.

6. LOGPROBS_ACCEPT_TIMEOUT can kill the import

It is parsed at module level with an unguarded float() and no range check. COLI_LOGPROBS_ACCEPT_TIMEOUT=30s makes import openai_server die with a ValueError traceback before anything can print a diagnostic, and it takes down every importer of the module. 0 or a negative value puts accept_deadline at or before time.monotonic(), so every logprobs request 503s on the first idle poll against a healthy engine. Compare _SALVAGE, the only other module-level env read, which cannot raise.

7. One test is racy and fails deterministically off CI

test_client_disconnect_mid_batch_stops_further_submits closes the socket and releases the blocked engine in the same instant, so the server runs all 8 members before the kernel delivers the RST: client_disconnected() is False on all 8 checks, len(engine.calls) == 8, and disconnect_observed.wait(5) times out. Reproduced 4 of 4 runs; a 0.5 s sleep between sock.close() and resume.set() makes it pass with calls == 3.

CI is green on a2c42bc3, all 28 checks, so this is timing that happens to go the right way on those runners. Please make the test wait for the disconnect to be observable rather than depend on scheduling.

8. Smaller things, all in c/openai_server.py

  • In the ECHO dispatcher arm, pos = int(fields[3]) raises a bare ValueError instead of the named RuntimeError the neighbouring size/terminator/tail checks use. It lands in _dispatch_stdout's blanket handler and fails every concurrent request with invalid literal for int(). It also makes _order_echo_records' own isinstance(pos, int) guard unreachable, so that validation is at the wrong layer.
  • batch_completion.submit_one is a copy of generation()'s non-streaming single-prompt block rather than a call into a shared helper, and the copies have already diverged: generation() applies split_thinking_reply and the tool-call parse and sends inside the admission, submit_one does neither. Any fix to the stop/trim/echo interaction above has to be made twice or it only fixes one endpoint shape.
  • One flag, supports_logprobs_echo, gates two unrelated capabilities: the U7a numeric channel and pre-tokenized token-id intake. They are separate keys with separate fields in coli_submit_ext. Two booleans set from the same arch == "glm" today costs one line and removes the conflation. The token-id refusal message also hardcodes "glm" instead of reading the flag it just checked.
  • The GRPP/GRPG/GRPS/GRPE arms and the group_score 400 add wire handling for a protocol docs/api.md states no engine in this tree emits. The arms guess at field counts, and the group_score guard is scoped to /v1/completions only, so the same opt-in is silently accepted on chat and messages. Please drop all five until the channel exists: the dispatcher's existing else: raise RuntimeError("invalid engine response") is already the right fail-closed behavior.
  • _write_all re-slices data[written:] every iteration, which is quadratic on exactly the large IMAGE frames it was written for, while holding write_lock. view = memoryview(data) once fixes it in the same three lines.
  • raw = [(data, record) for data, record in _order_echo_records(prompt_records)] rebuilds every tuple for nothing. And the if isinstance(member, str) filter inside the sum sits in a branch that already proved every member is a string, so it is an unconditional under-count waiting to happen rather than a guard.
  • For chat with thinking enabled or tools, logprobs.content describes tokens message.content does not contain, and nothing in the response marks it. It is under "Known limitations" in the docs, but a client aligning the two has no way to detect it at runtime.

9. Two process items

The branch needs a rebase on dev. The conflict is small: one region in c/tests/test_openai_server.py, where your test_seed_no_longer_rejected_by_generation_options and the test_tool_choice_function_that_is_not_an_object_is_a_400 that landed in #1598 were added at the same insertion point. Keeping both is the whole resolution. c/openai_server.py merges clean.

All 11 commits carry a Co-Authored-By trailer. This project does not take those, so the history needs rewriting before merge.

Happy to look again as soon as the substantive items are in. The parts that matter most to me are 1, 2 and 3.

@monotophic

Copy link
Copy Markdown
Contributor Author

Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic

@JustVugg thanks for the review — all nine items are being taken, with 1, 2 and 3 first and item 4 the way you preferred (alignment by raw-stream span, not prefix match).

My apologies for this being slow, I thought it would be quick to turn the package around with your items addressed, but I tried to keep it as a monolithic PR too far into the process AND the loss of our summer subscription subsidy meant I was working primarily with Opus 5 and we simply do not understand each other as well as needed to keep this package on track and up to standards. It was a risky tangle by Monday AM so I took a pause until fresh Fable tokens were available to sort it all out.

This branch grew well past the shape you praised, so rather than push that, we are re-cutting it into smaller pieces: seed accepted-and-ignored and the checked engine-write path each as their own small PR; this PR narrowed to logprobs/echo/top_logprobs with the span alignment; the array and token-id prompt intake, and a separate proposal for a full-stream logprobs field, each as their own PR after this one lands.

Nothing is pushed yet. Each piece will sit directly on current dev with no Co-Authored-By trailers.

Thanks again, I am working on making this worth your review time.

The stop filter, the thinking split and the tool-call parse only ever delete
characters, so each can report the surviving intervals of its own input.
Composing those maps turns "where did this token's characters end up" into a
lookup instead of a prefix match against a rewritten string. Behaviour and
output are unchanged; nothing reads the maps yet.
…ndpoints

Each endpoint takes its own OpenAI shape and refuses the other's by name; 0,
false and null normalise to absent. Records are located in the returned text by
raw-stream span, so "".join(tokens) is the text and logprobs.content joins to
message.content. The channel is requested only from a glm engine, engine-side
faults carry their own code, and a plain request's SUBMIT header is unchanged.
Request shapes and the zero/false/null rules for both endpoints, what the
arrays describe, and the named engine-side refusals. The capture battery's
logprobs case is renamed: it is served now, not refused.
@monotophic monotophic changed the title OpenAI API Instrumentation (e.g. seed, logprobs, echo, an array prompt) api: logprobs, top_logprobs and echo on the OpenAI endpoints, aligned by raw-stream span Sep 23, 2026
@monotophic monotophic closed this Sep 23, 2026
@monotophic monotophic reopened this Sep 23, 2026
@monotophic

Copy link
Copy Markdown
Contributor Author

@JustVugg, I closed/reopened this PR because the CI / Sanitizers (ASan + UBSan) job ran for 48 minutes and then GitHub annotated it: "The hosted runner lost communication with the server." This was at 28/29 passed checks.

I know that this is extra CI load, if you would rather I not cycle a PR like this to re-run hung CI, I can post a comment instead when I get this kind of fail. I have gotten hung CI processes on a couple of my pushes recently and they have cleared without requiring repair of the PR itself.

@monotophic

monotophic commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic

Pushed. This PR is now the logprobs work alone, on current dev as three commits, and the body carries
a table mapping each of your items to what changed and to the test that fails without it. It is about
the size of the diff you reviewed. We built a branch that answered the review, but it had grown to about twice the size,
so we cut the material into five parts. Once the first three land (#1353, #1720, and #1721), the other two follow.

Your echo: true, logprobs: 1, stop: ["lo"] case runs as a test now and fails on dev: the body is
promptHel, and the arrays say ["pr", "ompt", "Hel"].

Two things to highlight:

  • Item 1: your three records now keep two of three through the endpoint — <|user|> is
    dropped, because the client never received it, and the two that were received join back to
    message.content exactly. We have not reproduced that leading marker on a live glm engine, so the
    fix is covered by tests that drive the endpoint with your records, and we are not claiming a cure we
    never watched happen. Your reading of the default path is correct.
  • Item 2: the ACCEPT timeout is gone rather than relocated. A logprobs request now waits exactly as
    any request waits, so nothing frees an admission before the engine's turn ends — the case you
    described cannot arise. The residue is the one you would expect: behind a cold prefill the request
    waits, quietly, for as long as the prefill takes; the body says so and offers a bounded wait as its
    own PR. The same rule covers the faults this change names — a malformed numeric tail or a bad ECHO
    position fails that one request, its admission is held to the engine's terminal frame, and the
    dispatcher keeps serving.

/v1/brio is untouched: no line of its path is in the diff, and its header keeps the field it has
always carried.

Happy to look at anything you want changed.

@monotophic monotophic closed this Sep 23, 2026
@monotophic monotophic reopened this Sep 23, 2026
@monotophic monotophic closed this Sep 23, 2026
@monotophic monotophic reopened this Sep 23, 2026
@monotophic
monotophic requested a review from JustVugg September 24, 2026 07:07

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants