Skip to content

llama-server : make chat logprobs response OpenAI-compatible - #113

Merged
jimw567 merged 11 commits into
gfx11from
jamesong.aiesw-41278-logprobs-tools-stream
Oct 31, 2026
Merged

jimw567 merged 11 commits into
gfx11from
jamesong.aiesw-41278-logprobs-tools-stream

Conversation

@Json288

@Json288 Json288 commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Overview

This pr targets llama-server's completion and chat functionality. Replaces the stale OAI-incompatible comment.

The chat logprobs response is already OAI compatible, so no change is made to the response shape.

Behavior Changes

  • Removed the throw when stream + tool + logprobs is requested.
  • The streaming path now mirrors the existing non-streaming behavior — it emits per-token logprobs for all tokens except those of a matched stop string.
  • Streaming used to silently drop logprobs across chunks. They are now queued and flushed onto the next chunk with content.
  • Streaming used to drop text withheld as a possible stop-string start or an incomplete UTF-8 character, when a limit such as max_tokens is hit. It's now sent in the last chunk.
  • Fixed dropping logprobs for multi-token UTF-8 character and for partial stop string match that then diverges.
  • For matched stop strings, their tokens now have no entries in logprobs.
  • Fixed non-streaming trim that cut wrong number of entries for stop strings.

Additional information

N/A

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes. AI was used when planning and drafting the pr.

…est; remove the OAI incompatible comment

Signed-off-by: jamesong <jamesong@amd.com>
@Json288
Json288 requested a review from jimw567 September 22, 2026 21:34
@Json288
Json288 marked this pull request as ready for review September 22, 2026 22:03
@Json288
Json288 requested a review from Annieren October 5, 2026 22:07
@jimw567

jimw567 commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Shape compatibility is not API compatibility. The contract a client depends on is that logprobs cover the generated tokens. The repo already encodes that invariant explicitly: test_logprobs_stream asserts

assert aggregated_text == output_text

i.e. concatenating logprobs.content[].token across the stream reproduces the completion. That assertion is the compatibility test. The new test drops it and substitutes assert seen_logprobs — "at least one entry appeared anywhere." That weakening is the tell: the stricter assertion would fail, because of the empty-deltas drop we traced. A client summing logprobs to score a tool call gets a sum over an unknown subset, with no gap marker.

So on the goal as stated: the request is now accepted (good), the shape is right (was already), and the payload is incomplete in exactly the configuration the PR enables.

One thing that makes this cheap to fix. send_final_response already populates probs_output with the complete token vector even in stream mode (server-context.cpp:2140-2143) — and to_json_oaicompat_chat_stream never reads it (verified: no reference to probs_output or logprobs anywhere in that function, server-task.cpp:462-526). The full data is being assembled and thrown away. Emitting it on the final chunk, or synthesizing a logprobs-only chunk when deltas is empty, closes the gap without new plumbing.

Separate pre-existing compat gap, worth noting but not this PR's job: top_logprobs has no range validation (server-common.cpp:1409 — json_value(body, "top_logprobs", 20) passes straight through). OpenAI rejects values outside 0-20 with a 400; here top_logprobs: 100 is silently accepted. Pre-existing, but if the goal is auditing OpenAI compatibility it belongs on the list.

…tly dropped when a chat-msg diff is empty

Signed-off-by: jamesong <jamesong@amd.com>
Signed-off-by: jamesong <jamesong@amd.com>
@Json288

Json288 commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator Author

Shape compatibility is not API compatibility. The contract a client depends on is that logprobs cover the generated tokens. The repo already encodes that invariant explicitly: test_logprobs_stream asserts

assert aggregated_text == output_text

i.e. concatenating logprobs.content[].token across the stream reproduces the completion. That assertion is the compatibility test. The new test drops it and substitutes assert seen_logprobs — "at least one entry appeared anywhere." That weakening is the tell: the stricter assertion would fail, because of the empty-deltas drop we traced. A client summing logprobs to score a tool call gets a sum over an unknown subset, with no gap marker.

So on the goal as stated: the request is now accepted (good), the shape is right (was already), and the payload is incomplete in exactly the configuration the PR enables.

One thing that makes this cheap to fix. send_final_response already populates probs_output with the complete token vector even in stream mode (server-context.cpp:2140-2143) — and to_json_oaicompat_chat_stream never reads it (verified: no reference to probs_output or logprobs anywhere in that function, server-task.cpp:462-526). The full data is being assembled and thrown away. Emitting it on the final chunk, or synthesizing a logprobs-only chunk when deltas is empty, closes the gap without new plumbing.

Separate pre-existing compat gap, worth noting but not this PR's job: top_logprobs has no range validation (server-common.cpp:1409 — json_value(body, "top_logprobs", 20) passes straight through). OpenAI rejects values outside 0-20 with a 400; here top_logprobs: 100 is silently accepted. Pre-existing, but if the goal is auditing OpenAI compatibility it belongs on the list.

Updates according to the review:

  1. resolved the dropped logprobs by holding logprobs in a queue (pending_probs) and flushing them when an actual diff takes place or when sending the final chunk.
  2. fixed the problematic test to a count-based check (total_logprob_entries == completion_tokens) and switched the test model for consistent tool call
    what about this

@Json288
Json288 removed the request for review from Annieren October 16, 2026 00:30
@jimw567

jimw567 commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Please add the following tests

Mode Candidates Required assertions
Streaming 5 Actual tool call; valid logprob schema; exactly 5 candidates per token; token completeness
Streaming 100 Actual tool call; valid logprob schema; exactly 100 candidates per token; token completeness
Non-streaming 5 Actual tool call; valid logprob schema; exactly 5 candidates per token
Non-streaming 100 Actual tool call; valid logprob schema; exactly 100 candidates per token

Each candidate must include an integer token id. An actual tool call means checking the function name, valid assembled JSON arguments, and finish_reason: "tool_calls".

…ool calls

Signed-off-by: jamesong <jamesong@amd.com>
@Json288

Json288 commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Please add the following tests

Mode Candidates Required assertions
Streaming 5 Actual tool call; valid logprob schema; exactly 5 candidates per token; token completeness
Streaming 100 Actual tool call; valid logprob schema; exactly 100 candidates per token; token completeness
Non-streaming 5 Actual tool call; valid logprob schema; exactly 5 candidates per token
Non-streaming 100 Actual tool call; valid logprob schema; exactly 100 candidates per token
Each candidate must include an integer token id. An actual tool call means checking the function name, valid assembled JSON arguments, and finish_reason: "tool_calls".

Requested tests are added. Replaced the old streaming-only test_logprobs_with_tools_stream() with test_logprobs_with_tools, parametrized into 4 cases matching the test matrix. Each case checks: actual tool call, valid logprob schema, exactly N candidates per token (5 or 100 as requested), token completeness (streaming only).

The tests are locally verified and all pass.

@jimw567

jimw567 commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

A couple of things:

  • The four new tests pass, but token completeness still fails when a tool argument contains a character represented by multiple token byte pieces.

Using the PR's Llama-3.2-1B-Instruct Q4_K_M model with logprobs: true and top_logprobs: 5:

  • U+1F9EA: 19 generated tokens, but only 17 logprob entries.
  • U+1F600: 18 generated tokens, but only 17 logprob entries.

Both streaming and non-streaming return correct tool arguments and finish_reason: "tool_calls". The problem is the logprob payload: some generated tokens have no entry, and the completing token's probability is attached to the merged character rather than its individual byte piece. A client therefore cannot reliably score the complete generated tool call.

This is a pre-existing issue that also affects the streaming-tool configuration newly enabled by this PR. The current ASCII-only test arguments do not exercise it. The expected behavior is complete, correctly attributed per-token logprobs even when a character spans multiple tokens.

  • The new schema checks do not validate bytes on each top_logprobs candidate (tools/server/tests/unit/test_tool_call.py:329-332). They check only integer id, presence of token, and non-positive logprob.

Consequently, missing or malformed candidate bytes would still pass all four tests. The requested valid-schema coverage should include that field.

Signed-off-by: jamesong <jamesong@amd.com>
Signed-off-by: jamesong <jamesong@amd.com>
@Json288

Json288 commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

A couple of things:

  • The four new tests pass, but token completeness still fails when a tool argument contains a character represented by multiple token byte pieces.

Using the PR's Llama-3.2-1B-Instruct Q4_K_M model with logprobs: true and top_logprobs: 5:

  • U+1F9EA: 19 generated tokens, but only 17 logprob entries.
  • U+1F600: 18 generated tokens, but only 17 logprob entries.

Both streaming and non-streaming return correct tool arguments and finish_reason: "tool_calls". The problem is the logprob payload: some generated tokens have no entry, and the completing token's probability is attached to the merged character rather than its individual byte piece. A client therefore cannot reliably score the complete generated tool call.

This is a pre-existing issue that also affects the streaming-tool configuration newly enabled by this PR. The current ASCII-only test arguments do not exercise it. The expected behavior is complete, correctly attributed per-token logprobs even when a character spans multiple tokens.

  • The new schema checks do not validate bytes on each top_logprobs candidate (tools/server/tests/unit/test_tool_call.py:329-332). They check only integer id, presence of token, and non-positive logprob.

Consequently, missing or malformed candidate bytes would still pass all four tests. The requested valid-schema coverage should include that field.

Applied two fixes as requested:

  1. fix previously discarded logprobs for UTF-8 multi-token characters
  2. apply stricter bytes check in tool-call logprobs tests

Comment thread tools/server/server-context.cpp Resolved
… match branch and add unit test

Signed-off-by: jamesong <jamesong@amd.com>
…nit test

Signed-off-by: jamesong <jamesong@amd.com>
… dropping held text when generation ends; edit unit tests accordingly

Signed-off-by: jamesong <jamesong@amd.com>
Signed-off-by: jamesong <jamesong@amd.com>
@Json288

Json288 commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author
  • Partial match that diverges: the token's text is kept and its entry is emitted once the text is sent.
  • Real stop match: the stop-string tokens get no entries, and a token the stop string starts in keeps only its
    visible part. This now behaves the same in streaming and non-streaming, and the old count-based non-streaming
    trim is removed.

@jimw567 jimw567 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good

@jimw567
jimw567 merged commit 1627692 into gfx11 Oct 31, 2026
9 checks passed
@jimw567
jimw567 deleted the jamesong.aiesw-41278-logprobs-tools-stream branch October 31, 2026 17:33
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