[tinker] Return prompt and top-k prompt logprobs from sample requests - #1945
Open
pcmoritz wants to merge 5 commits into
Open
[tinker] Return prompt and top-k prompt logprobs from sample requests#1945pcmoritz wants to merge 5 commits into
pcmoritz wants to merge 5 commits into
Conversation
…uests `sample` requests carrying `prompt_logprobs` / `topk_prompt_logprobs` came back with both fields unset, which the Tinker SDK surfaces as "Tinker response did not include prompt_logprobs". Three gaps, all in the glue between the API layer and the backends: - `api.py` accepted `topk_prompt_logprobs` and dropped it when building `types.SampleInput`, so it never reached a backend. A positive top-k now also implies `prompt_logprobs` (both come off the same prompt forward pass). - The SkyRL-Train backend never put `include_prompt_logprobs` / `topk_prompt_logprobs` in the sample body, so vLLM was never asked for them, and then read the result as a list-of-lists when `RemoteInferenceClient.sample` returns flat per-prompt-token lists -- position 0 (always None) would have been returned as the whole field. Only the first sample of each request asks for prompt logprobs now, since all `num_samples` samples share one prompt. - `types.SampleOutput.prompt_logprobs` was typed `list[float]`, which rejects the None at position 0. The two forwarding clients in `extra/` also gain prompt-logprob support via vLLM's `/v1/completions`, and the vLLM -> Tinker conversion is now shared (`convert_vllm_prompt_logprobs`) so the three paths cannot drift. The JAX backend returns an explicit error for `topk_prompt_logprobs`: tx's generator only produces the prompt tokens' own logprobs, so there is no per-position distribution to build top-k from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for requesting top-k prompt logprobs (topk_prompt_logprobs) alongside standard prompt logprobs. It adds a helper function convert_vllm_prompt_logprobs to standardize the conversion of vLLM's prompt logprobs into Tinker's response format. The SkyRL-Train backend is updated to forward and parse these top-k logprobs, while the JAX backend is updated to explicitly reject requests with topk_prompt_logprobs > 0 as it is unsupported. Unit tests have also been added to verify this new functionality. I have no additional feedback to provide.
bvolpato
approved these changes
Aug 2, 2026
bvolpato
left a comment
bvolpato
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Nice end-to-end handling across API, SkyRL-Train, and both forwarding clients. Nonblocking follow-up: reject unsupported JAX topk_prompt_logprobs before generation, so known-failing requests avoid unnecessary work. Not blocking this PR.
The JAX-backed test in tests/tinker/test_api.py can only assert the flat prompt_logprobs field -- tx's generator has no per-position distribution, so topk_prompt_logprobs is rejected there. Add the full contract to the existing megatron + vLLM e2e module instead of a new server fixture: - num_samples=3 with include_prompt_logprobs: three sequences and one shared flat list of exactly len(prompt_tokens) entries. This is where the bug bit -- only the first sample asks vLLM for prompt logprobs and the result is read back from that same sample, and a list-of-lists misread shows up as a length mismatch. - topk_prompt_logprobs with include_prompt_logprobs deliberately omitted, so a positive top-k implying prompt logprobs is covered too. Each position returns at most k descending (token_id, logprob) pairs, and where the prompt's own token is inside the top-k its logprob must match the flat list -- that catches the two fields drifting apart by a position. - Neither field requested: both stay unset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rature test_sample_prompt_logprobs asked vLLM for num_samples=3 with temperature=0.0, and vLLM's completions endpoint rejects that outright: vLLM /v1/completions returned 400: "n must be 1 when using greedy sampling, got 3." The test died on that first sample call, so none of the prompt-logprob assertions -- including the top-k block that only this module can reach -- ever ran. The rest of the module pairs greedy with num_samples=1, which is why the combination hadn't been hit before. Give the multi-sample case its own params at temperature=1.0 (seeded, so the run stays reproducible) and leave the two num_samples=1 calls greedy. Prompt logprobs come off the prompt forward pass and don't depend on how the continuation is drawn, so the coverage this was written for -- three sequences over one shared prompt, flat list read back from the first sample -- is unchanged. Verified on 8xH100 (megatron + vLLM, tiny-Qwen3ForCausalLM): the test now passes end to end in 4:40, exercising the top-k assertions for the first time. Before this it failed in 6:48 without asserting anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
erictang000
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
samplerequests carryingprompt_logprobs/topk_prompt_logprobscame back with both fields unset, which the Tinker SDK surfaces as "Tinker response did not include prompt_logprobs".Three gaps, all in the glue between the API layer and the backends:
api.pyacceptedtopk_prompt_logprobsand dropped it when buildingtypes.SampleInput, so it never reached a backend. A positive top-k now also impliesprompt_logprobs(both come off the same prompt forward pass).include_prompt_logprobs/topk_prompt_logprobsin the sample body, so vLLM was never asked for them, and then read the result as a list-of-lists whenRemoteInferenceClient.samplereturns flat per-prompt-token lists -- position 0 (always None) would have been returned as the whole field. Only the first sample of each request asks for prompt logprobs now, since allnum_samplessamples share one prompt.types.SampleOutput.prompt_logprobswas typedlist[float], which rejects the None at position 0.The two forwarding clients in
extra/also gain prompt-logprob support via vLLM's/v1/completions, and the vLLM -> Tinker conversion is now shared (convert_vllm_prompt_logprobs) so the three paths cannot drift.The JAX backend returns an explicit error for
topk_prompt_logprobs: tx's generator only produces the prompt tokens' own logprobs, so there is no per-position distribution to build top-k from.