Skip to content

fix: cache bad_words tokenization to avoid 'Already borrowed' errors under concurrency - #45522

Open
Oxygen56 wants to merge 2 commits into
vllm-project:mainfrom
Oxygen56:fix/issue-45445
Open

Oxygen56 wants to merge 2 commits into
vllm-project:mainfrom
Oxygen56:fix/issue-45445

Conversation

@Oxygen56

Copy link
Copy Markdown
Contributor

Summary

Fixes #45445RuntimeError: Already borrowed when using bad_words with concurrent OpenAI serving requests.

Root Cause

SamplingParams.update_from_tokenizer() calls tokenizer.encode() for each bad_word on every request. When many concurrent requests share the same bad_words list (a common workload pattern, e.g. RL), these redundant encode calls race with prompt tokenization on the same HF fast tokenizer pool. This can still trigger "Already borrowed" even after the thread-safe tokenizer wrapper in #41181 was merged.

Fix

Cache tokenized bad_word results on the tokenizer object (lifetime-bound to the tokenizer). When a bad_word has been tokenized before, the cached result is reused, avoiding the redundant tokenizer.encode() call entirely.

  • Cache key: (bad_word, add_prefix_space) tuple
  • Cache storage: tokenizer._bad_words_token_cache dict (created lazily)
  • Thread-safety: Python dict operations under GIL are safe for this read-heavy workload pattern

Validation

The reporter confirmed that locally caching bad_words IDs inside update_from_tokenizer() eliminated all "Already borrowed" failures across their A/B hammer test:

  • Without cache: intermittent HTTP 500s with RuntimeError: Already borrowed
  • With cache: 0 HTTP 500s across the same hammer rounds

Test Plan

  • No functional change to tokenization logic — same result, just cached
  • Cache lifetime is bound to the tokenizer (cleaned up when tokenizer is replaced)
  • Backward compatible — _bad_words_token_cache attribute is lazily created if absent

@Oxygen56
Oxygen56 force-pushed the fix/issue-45445 branch 2 times, most recently from 72f3f7e to 62d70f2 Compare June 13, 2026 16:00
@Oxygen56

Copy link
Copy Markdown
Contributor Author

The pre-run-check failure is due to the repo policy requiring either 4 merged PRs or the 'ready' label (I have 2 merged PRs). Could a maintainer add the 'ready' label to trigger CI? The code change itself should pass formatting checks.

Note: the readthedocs failure appears unrelated to this PR's changes — it's a docs build environment issue.

@mergify

mergify Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @Oxygen56.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Jul 29, 2026
…der concurrency

Signed-off-by: Willow Lopez <100782273+Oxygen56@users.noreply.github.com>
@Oxygen56

Oxygen56 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest main and resolved the sampling_params.py conflict in 755829d55, preserving the current VLLMValidationError validation behavior.

Verification:

  • python3 -m py_compile vllm/sampling_params.py -> passed
  • git diff --check upstream/main...HEAD -> passed

The branch is ready for CI/review; the repository ready label remains maintainer-controlled.

@mergify mergify Bot removed the needs-rebase label Aug 1, 2026
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.

[Bug]: RuntimeError: Already borrowed still occurs with bad_words despite thread-safe tokenizer wrapper

1 participant