Fix interception proxy capturing 0 token ids on current vLLM - #1029
Fix interception proxy capturing 0 token ids on current vLLM#1029sergiopaniego wants to merge 1 commit into
Conversation
burtenshaw
added
bug
Cursor
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # instead expose a separate ``token_id`` key. Accept both. | ||
| token_id = entry.get("token_id") | ||
| if token_id is None and token.startswith("token_id:"): | ||
| token_id = token.split(":", 1)[1] |
There was a problem hiding this comment.
Optional hardening (non-blocking). If an upstream build ever emits a malformed prefix — e.g. "token_id:" (empty suffix) or a non-numeric suffix — the int(token_id) call two lines below will raise ValueError and abort building the turn record. vLLM's --return-tokens-as-token-ids always produces a valid integer here, and this matches the existing behavior for the separate token_id key, so it's low priority. A small guard (e.g. wrap the int(...) in try/except ValueError, or check the suffix .isdigit()) would make capture resilient to unexpected upstream output rather than dropping the whole turn.
vLLM with --return-tokens-as-token-ids encodes the token id in the logprobs "token" field as "token_id:<int>" rather than a separate "token_id" key, so the capture read zero token ids. Every env that trains on the captured trace (opencode, pi, ...) then loses the token ids GRPO needs. Parse the id from the token field. Adds a unit test.
d7cea68 to
d6227f3
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6227f3. Configure here.
| tokens.append(token) | ||
| # vLLM (--return-tokens-as-token-ids) returns the id in the token field as "token_id:<int>". | ||
| if token.startswith("token_id:"): | ||
| token_ids.append(int(token.split(":", 1)[1])) |
There was a problem hiding this comment.
Legacy token_id key support dropped
Medium Severity
_build_turn_record only parses ids from a token value with the token_id: prefix and no longer reads a separate token_id field. Builds that still emit that key end up with empty completion_token_ids, which breaks GRPO training on those traces. The PR claims both formats are supported, but the legacy path and its test are missing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d6227f3. Configure here.
|
Closing this after investigating whether the empty The TRL rollout worker's For the record, vLLM's |
What does this PR do?
The interception proxy (
opencode_env/sandbox/interception.py) captures per-token ids and logprobs from the upstream vLLM response so GRPO can train on the exact tokens the policy emitted. On current vLLM this capture returned zero token ids.vLLM with
--return-tokens-as-token-idsencodes the id in the logprobstokenfield as"token_id:<int>"rather than a separatetoken_idkey, which is what the capture read. Socompletion_token_idscame back empty whileper_token_logpswere fine, and every env that trains on the captured trace (opencode, pi, and the in-progress Claude Code env) loses the token ids GRPO needs.The fix parses the id from the
tokenfield.Validation
tests/envs/test_interception_capture.pycovers the vLLM prefixed-token format and the plain-OpenAI (no ids) case.[40, 3278, 1855, ...]) end to end, without it the trace had 0.Note
Independent of the sandbox-to-core move (#1022). When that lands, the same fix needs to travel with the moved
interception.py.AI-assisted.