Skip to content

feat(client): pass through sampling-mask logprobs and by-handle payload segments - #170

Closed
faresobeid wants to merge 7 commits into
mainfrom
feat/top-logprobs-v2
Closed

faresobeid wants to merge 7 commits into
mainfrom
feat/top-logprobs-v2

Conversation

@faresobeid

@faresobeid faresobeid commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

renderers.client.generate() passes two more optional fields of prime-rl's /inference/v1/generate response through, next to sampling_mask. Pure pass-through: no request-side change, no parsing. Servers that do not send them (stock vLLM, current prime-rl main) give None.

  • sampling_mask_logprobs: the sampler's logprob at each sampling-mask id, packed base64 float32, parallel to the packed sampling_mask ids.
  • payload: by-handle segments {field, file, offset, pos, rows, dtype, shape} of a shared file that the inference server writes instead of returning routed_experts / sampling_mask inline.

Validation

uv run pytest tests/test_client.py (104 passed, 6 skipped), ruff check, ruff format --check, ty check renderers (warnings only, as on main); CI green.

Dependencies

Note

Expose sampling_mask_logprobs and payload in client generate response

  • generate in renderers/client.py now reads sampling_mask_logprobs from the inference response choice and returns it alongside sampling_mask.
  • Also returns the choice's payload, which holds by-handle side-array metadata for data stored by the inference server instead of inline.
  • Documents the packed float32 log probabilities emitted when score centering is enabled.
  • Risk: callers of generate see two new keys in the returned dictionary; existing fields are unchanged.

Macroscope summarized 424ba10.


Note

Low Risk
Additive response pass-through only; no request, validation, or behavior changes when engines omit the new fields.

Overview
Extends generate() so callers get two more optional fields from prime-rl’s /inference/v1/generate response, with no request or parsing changes.

sampling_mask_logprobs is forwarded from the choice (packed float32 logprobs parallel to sampling_mask ids when score centering is enabled). payload is forwarded for by-handle segments pointing at a shared file when the server offloads routed_experts / sampling_mask instead of returning them inline. Both appear in the returned dict and docstring; absent keys become None, same as existing optional pass-throughs like sampling_mask.

Reviewed by Cursor Bugbot for commit 424ba10. Bugbot is set up for automated code reviews on this repo. Configure here.

@faresobeid faresobeid changed the title feat(client): opt-in per-token top-k logprobs feat(client): pass through sampling-mask logprobs Oct 3, 2026
@faresobeid
faresobeid marked this pull request as ready for review October 3, 2026 11:52
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The PR is a small, backwards-compatible pass-through of optional server metadata and does not materially change existing request or response behavior. Human review is still warranted because two unresolved comments identify concrete reasoning-text correctness issues elsewhere in the PR branch.

Notes:

  • Macroscope's correctness review did not run, so approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@faresobeid
faresobeid force-pushed the feat/top-logprobs-v2 branch 3 times, most recently from 01c9cce to 1cd3446 Compare October 6, 2026 17:19
@faresobeid
faresobeid changed the base branch from main to perf/packed-completion-logprobs October 6, 2026 17:19
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 6, 2026
@faresobeid
faresobeid force-pushed the perf/packed-completion-logprobs branch from e134d56 to e161362 Compare October 8, 2026 14:35
@faresobeid
faresobeid force-pushed the feat/top-logprobs-v2 branch from 1cd3446 to ea01237 Compare October 8, 2026 14:35
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 12, 2026 18:47

Dismissing prior approval to re-evaluate d3a1af0

@faresobeid faresobeid changed the title feat(client): pass through sampling-mask logprobs feat(client): pass through sampling-mask logprobs and by-handle payload segments Oct 12, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 12, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 14, 2026 14:34

Dismissing prior approval to re-evaluate 030b208

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 030b208. Configure here.

Comment thread renderers/reasoning.py
close == -1, text[start : close if close != -1 else len(text)], by_tool
close == -1,
text[start : close if close != -1 else len(text)].replace(open_marker, ""),
by_tool,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reasoning text drops think markers

Medium Severity

scan_reasoning already skips the leading <think> opener, then deletes every later open_id and every <think> substring before decoding what remains. Reasoning that mentions the tag, or a truncated </think>, loses those characters, and neighboring tokens can merge into different text.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 030b208. Configure here.

Comment thread renderers/parsing.py
(reasoning or "").rstrip()
if parsed.boundary.is_open
else (reasoning or "").rstrip() or None
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DeepSeek reasoning boundary whitespace stripped

Medium Severity

parse_deepseek_v4 always rstrips reasoning and turns whitespace-only content into "". The newline the model emits before </think>, and a content channel that is only that separator, disappear from the parsed message.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 030b208. Configure here.

@faresobeid
faresobeid changed the base branch from perf/packed-completion-logprobs to main October 14, 2026 17:04
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.

1 participant