fix(output): prevent prompt-tail tokens leaking into streamed output_ids#584
fix(output): prevent prompt-tail tokens leaking into streamed output_ids#584minedec wants to merge 3 commits into
Conversation
minedec
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b06818c0a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if state.obj.stream: | ||
| state.logprobs_info = logprobs_info | ||
| state.output_ids.extend(recv_obj.decode_ids[i]) | ||
| state.output_ids.extend(recv_obj.output_ids[i]) |
There was a problem hiding this comment.
When a BatchTokenIDOut reaches this branch without output_ids populated, this new dereference raises before any streamed output is collected. BatchTokenIDOut.output_ids is still documented/typed as optional and only for --skip-tokenizer-init (io_struct.py:530), and the existing inline receiver helper defaults it to None while constructing inline frames from decode_ids only (for example test_inline_detokenizer_receiver.py:298-303). Please either make the producer/test contract require pure generated output_ids for inline detokenization or fall back/validate here so those valid frames do not crash.
Useful? React with 👍 / 👎.
lightseek-bot
force-pushed
the
jjd/fix_output_processor
branch
from
56f4fcb to
c31e947
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c31e947b5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.output_ids.extend(self._inline_output_ids(recv_obj, i)) | ||
| output_token_ids = state.output_ids[state.last_output_offset :] | ||
| state.last_output_offset = len(state.output_ids) |
There was a problem hiding this comment.
When two streaming inline frames are processed before _wait_one_response drains the collector, RequestOutputCollector._extend_sequence guesses cumulative-vs-delta by checking whether the pending ids are a prefix of the next value. This branch now emits pure generated deltas from BatchTokenIDOut.output_ids, so a normal repeated-token case such as first delta [42] and next delta [42] is misclassified as cumulative and the second token is dropped from the coalesced streamed output_ids. Please make this path unambiguously cumulative or otherwise tell the collector these values are deltas before switching to output_ids here.
Useful? React with 👍 / 👎.
lightseek-bot
force-pushed
the
jjd/fix_output_processor
branch
from
c31e947 to
f8b0309
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8b0309a16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pending["text"] = output["text"] | ||
|
|
||
| self._extend_sequence(pending, "output_ids", output.get("output_ids")) | ||
| self._append_sequence(pending, "output_ids", output.get("output_ids")) |
There was a problem hiding this comment.
In the raw-token path used when the tokenizer is absent/--skip-tokenizer-init, output_processor.py still emits cumulative state.output_ids.copy() whenever state.obj.stream is true but server_args.stream_output is false (the default). Because this line now blindly appends during collector coalescing, two pending frames such as [1] then [1, 2] are returned as [1, 1, 2] if _wait_one_response has not drained the first frame yet. Please keep the prefix-aware merge for cumulative producers or only use this append path for outputs known to be deltas.
Useful? React with 👍 / 👎.
Signed-off-by: lightseek-bot <243258330+lightseek-bot@users.noreply.github.com>
lightseek-bot
force-pushed
the
jjd/fix_output_processor
branch
from
f8b0309 to
28b76f4
Compare
Signed-off-by: lightseek-bot <243258330+lightseek-bot@users.noreply.github.com>
Signed-off-by: lightseek-bot <243258330+lightseek-bot@users.noreply.github.com>
2 participants
Summary
The inline-detokenizer branch of OutputProcessor incorrectly extended
state.output_idswithrecv_obj.decode_ids[i], which includes a 5-token prompt-tail context window (INIT_INCREMENTAL_DETOKENIZATION_OFFSET) needed by the incremental detokenizer. This caused the first 5 tokens of every streamed response to echo the prompt suffix (e.g. <|im_start|>assistant\n\n) as if they were freshly sampled output. Changed torecv_obj.output_ids[i]to match the BatchStrOut and raw-token branches, which already use the correct field containing only actually-sampled tokens.Test Plan