Skip to content

fix(browsecomp): accept both vLLM /tokenize response shapes - #2789

Merged
mjmikulski merged 2 commits into
mainfrom
mmikulski/browsecomp-tokenize-count-fallback
Aug 26, 2026
Merged

fix(browsecomp): accept both vLLM /tokenize response shapes#2789
mjmikulski merged 2 commits into
mainfrom
mmikulski/browsecomp-tokenize-count-fallback

Conversation

@mjmikulski

Copy link
Copy Markdown
Contributor

save_model_call_using_vllm_tokenize_endpoint reads len(response["tokens"]), which only exists on older vLLM builds; mainstream >=0.19.1 returns {"count": N, "max_model_len": ...} and omits the token list unless token ids are requested, so the pre-call context-reset estimation dies with KeyError: 'tokens' on every sample. Prefer the explicit count, fall back to the token list, and fail loudly when neither is present.

What does this PR do?

Checklist

  • I have read the contributing guidelines.
  • The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs.
  • Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable).
  • Pre-commit checks pass locally (pre-commit run --all-files) (so CI lint/format/copyright pass).
  • All commits have DCO sign-off (git commit -s) (so the DCO check passes).

mjmikulski and others added 2 commits August 26, 2026 13:09
save_model_call_using_vllm_tokenize_endpoint reads len(response["tokens"]),
which only exists on older vLLM builds; mainstream >=0.19.1 returns
{"count": N, "max_model_len": ...} and omits the token list unless token
ids are requested, so the pre-call context-reset estimation dies with
KeyError: 'tokens' on every sample. Prefer the explicit count, fall back
to the token list, and fail loudly when neither is present.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: MJ Mikulski <mmikulski@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: MJ Mikulski <mmikulski@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mjmikulski

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

SHIP — LGTM, no reliability concerns.

Focused robustness fix for _count_prompt_tokens: vLLM's /tokenize response omits the tokens list unless token ids are requested (mainstream >=0.19.1 returns {"count": N, ...}), so the old len(tokenize_response["tokens"]) would KeyError against current vLLM and silently break context-reset decisions. New helper prefers the explicit count, falls back to len(tokens) for older builds, and raises loudly (not a silent 0) when neither is present — the right failure mode for a value feeding reset logic. Test asserts all four cases including count-wins-over-tokens and the loud-failure path. No async, API-surface, or scoring concerns.

@mjmikulski
mjmikulski requested a review from ritugala August 26, 2026 11:37
Comment thread responses_api_agents/browsecomp_agent/app.py
@mjmikulski

Copy link
Copy Markdown
Contributor Author

/ok to test 6c3a546

@mjmikulski
mjmikulski enabled auto-merge (squash) August 26, 2026 13:03
@mjmikulski
mjmikulski merged commit e985256 into main Aug 26, 2026
31 checks passed
@mjmikulski
mjmikulski deleted the mmikulski/browsecomp-tokenize-count-fallback branch August 26, 2026 13:24
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