api: logprobs, top_logprobs and echo on the OpenAI endpoints, aligned by raw-stream span - #1353
monotophic wants to merge 6 commits into
Conversation
Integrazione PR upstream: CUDA Qwen3.8 (JustVugg#1424), QLoRA (JustVugg#626), API logprobs/echo (JustVugg#1353)
497d59b to
2edcea6
Compare
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic Rebased onto current One file needed conflict resolution,
Re-verified on @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. |
|
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, One question before I merge, and it is the only default-path behaviour change in the diff. The 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 |
2edcea6 to
a2c42bc
Compare
|
Authored by Claude Fable 5.1 in Claude Code, analysis in partnership with @monotophic @JustVugg Thank you! Revised as requested. Pushed
|
JustVugg
left a comment
There was a problem hiding this comment.
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 bareValueErrorinstead of the namedRuntimeErrorthe neighbouring size/terminator/tail checks use. It lands in_dispatch_stdout's blanket handler and fails every concurrent request withinvalid literal for int(). It also makes_order_echo_records' ownisinstance(pos, int)guard unreachable, so that validation is at the wrong layer. batch_completion.submit_oneis a copy ofgeneration()'s non-streaming single-prompt block rather than a call into a shared helper, and the copies have already diverged:generation()appliessplit_thinking_replyand the tool-call parse and sends inside the admission,submit_onedoes 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 incoli_submit_ext. Two booleans set from the samearch == "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_score400 add wire handling for a protocoldocs/api.mdstates no engine in this tree emits. The arms guess at field counts, and thegroup_scoreguard is scoped to/v1/completionsonly, so the same opt-in is silently accepted on chat and messages. Please drop all five until the channel exists: the dispatcher's existingelse: raise RuntimeError("invalid engine response")is already the right fail-closed behavior. _write_allre-slicesdata[written:]every iteration, which is quadratic on exactly the large IMAGE frames it was written for, while holdingwrite_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 theif 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.contentdescribes tokensmessage.contentdoes 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.
|
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: Nothing is pushed yet. Each piece will sit directly on current 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.
a2c42bc to
73dc1f5
Compare
|
@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. |
|
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 Your Two things to highlight:
Happy to look at anything you want changed. |
…ngine writes (JustVugg#1721) under the logprobs work
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
devas 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/completionstakes the legacy integerlogprobs(0–32) withecho;/v1/chat/completionstakes booleanlogprobswithtop_logprobs. The server requests theengine's existing
logprobs=kchannel only from a glm engine and refuses the fields with a named400 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) == textand"".join(logprobs.content[*].token) == message.contenthold exactly, including for a token astopcut in half. A request that never asksfor logprobs writes the byte-identical
SUBMITheader; two tests pin that againstdev. A logprobsrequest normally forfeits prefix-cache reuse, because the channel re-prefills from position 0.
test_swallowed_leading_marker_keeps_the_other_two_recordsand its completions twintest_swallowed_leading_marker_offsets_index_the_returned_textEngine.generate()gains one keyword,gbytes_before_ext=False, and every existing caller's wire is unchangedSUBMITpins; a bounded wait is offered separatelyecho:true, logprobs:1, stop:["lo"]shape:test_echo_keeps_the_prompt_and_the_emitted_prefix(textpromptHel, offsets[0, 2, 6]); bytes:test_a_truncated_entrys_bytes_do_not_describe_the_withheld_textLogprobsStageCompositionTestmethods that assert each surviving entry's own logprob valueecho: nullandfalsenormalise to absent on both endpointstest_echo_null_normalizes_to_absent_both_endpoints,test_zero_false_null_normalize_to_absent,test_chat_echo_false_normalises_to_absent_over_httpengine_echo_position_malformed; the dispatcher keeps servingtest_a_malformed_echo_position_spares_a_request_that_is_in_flightCapabilitySplitIndependenceTestmethods andtest_the_capability_refusal_names_the_gate_not_the_engines_archtest_content_is_a_projection_of_the_stream_not_a_subset, which asserts the entry count before what is absentorigin/devdirectly, no merge commits, no trailersgit merge-base HEAD origin/dev=origin/dev; trailer count 0Changed by design against
dev. These, and only these, answer a named 400 wheredevanswered200: a non-boolean
echoon either endpoint;echo: trueon chat;echo: truewithoutlogprobson completions; an invalid
top_logprobswhilelogprobsis off; and alogprobsvalue that isfalsy but neither
falsenornull(0on chat;'',[],{}on either endpoint), whichdevignored by truthiness.
logprobs: 0on completions still answers 200, and a validtop_logprobswithlogprobsoff stays a no-op. The 400→200 shapes are the feature:logprobs1–32 on completions andlogprobs: trueon 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
codeandparam. A faulted turn writes oneSTOP, waits for the engine's own terminal frame with itsadmission held, and only then answers; a client cancellation still wins on the two terminal frames that report
nothing else, an
ERROR CANCELLEDor aDONE; if theSTOPitself cannot be written, the request answers theengine-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 fromc/; 149 of the head'stests 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.pyand
test_systemone_api.py: 360 passed, 153 subtests. Module wall time 24.7 → 26.1 s, no new test over0.30 s.
make check: 1512 tests, OK.On hardware. From a stacked build of this work on one glm engine (spark2, the
glm52_i3g64_probecontainer), 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 wassubstituted;
echo: nullwas served a 200 on both endpoints; k=32 returned 32 candidates per position with noduplicate; the plain path's SUBMIT header was byte-identical to the base binary's.