Skip to content

[misc] fix: Enable OpenAI streaming for HF tokenizers - #5739

Merged
yaoyu-33 merged 1 commit into
mainfrom
yuya/verified-bug-hunter-openai-streaming-tokenizer-20260822
Aug 22, 2026
Merged

yaoyu-33 merged 1 commit into
mainfrom
yuya/verified-bug-hunter-openai-streaming-tokenizer-20260822

Conversation

@yaoyu-33

Copy link
Copy Markdown
Contributor

Problem

The maintained Bridge OpenAI-compatible server rejects supported stream=true requests even when it loaded a real Hugging Face fast tokenizer.

build_tokenizer() stores the raw Hugging Face tokenizer at HFTokenizerAdapter._tokenizer. The pinned MCore streaming endpoints then treat that value as MCore's Hugging Face wrapper and look for _tokenizer.tokenizer. Standard fast tokenizers expose backend_tokenizer / internal _tokenizer, not .tokenizer, so both chat and text completion streaming return HTTP 400 before submitting generation:

ValueError: Streaming is currently supported only for Hugging Face fast tokenizers.

Non-streaming generation is unaffected. This is adjacent to the OpenAI adapter contract completed in #5091.

Fix

When the wrapped tokenizer does not already provide MCore's wrapper fields, expose:

  • tokenizer pointing to the real Hugging Face fast tokenizer;
  • include_special_tokens derived from the adapter's detokenization policy.

The second field keeps incremental streaming aligned with Bridge's normal special-token behavior; without it, construction succeeds but the EOS marker is emitted in the stream.

Regression evidence

An exact-source CPU reproducer passed a real PreTrainedTokenizerFast through the audited Bridge adapter into the pinned MCore incremental detokenizer.

  • Before: exit 1 at the false fast-tokenizer rejection shown above.
  • After, unchanged: exit 0; EOS produces no streamed text.

The added unit regression exercises that same cross-package contract with a real fast tokenizer.

Focused and adjacent CPU validation (isolated package-init harness; exact checkout source paths):

uv run --no-project --python 3.12 \
  --with pytest --with pytest-mock --with tokenizers --with transformers --with torch --with pillow \
  python -m pytest --confcutdir=tests/unit_tests/inference \
  tests/unit_tests/inference/test_tokenizer.py \
  tests/unit_tests/inference/vlm/test_vlm_inference_controller.py -q

21 passed

Static validation:

git diff --check
# passed

uv run --no-project --python 3.12 --with pre-commit==3.6.0 pre-commit run --all-files
# all configured hooks passed

The project-managed pre-commit invocation was blocked before hook execution because the pinned resiliency wheel requires a newer glibc platform; the isolated invocation above ran the repository's complete configured hook set.

Scope

This changes only the private inference tokenizer adapter and one CPU unit test. It does not change model weights, generation algorithms, dependencies, CI, public conversion APIs, or MCore source. No full-suite, GPU/model-quality, convergence, or performance validation is claimed.

Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 22, 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.

@yaoyu-33

Copy link
Copy Markdown
Contributor Author

/ok to test 120e850

@yaoyu-33 yaoyu-33 added area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer labels Aug 22, 2026
@claude

claude Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Light review - HFTokenizerAdapter streaming fix

The change makes HFTokenizerAdapter compatible with MCore HuggingFaceFastIncrementalDetokenizer (OpenAI streaming path) by populating tokenizer / include_special_tokens attributes, guarded by hasattr so existing values are preserved. Scope is tight and the test exercises the previously-broken construction path. A couple of concerns:

  1. (High / needs author confirmation) Attributes are set on the wrapped tokenizer, but the test feeds the adapter to the detokenizer. In init, 'tokenizer.tokenizer = tokenizer' and 'tokenizer.include_special_tokens = ...' mutate the wrapped HF tokenizer (self._tokenizer). But the new test constructs the detokenizer with the adapter: HuggingFaceFastIncrementalDetokenizer(adapter, []). The adapter has no getattr, so adapter.tokenizer / adapter.include_special_tokens are never set. If MCore reads these off the object it is constructed with, this fix targets the wrong object; if the test still passes, it is only via a getattr(..., default) fallback in MCore, making this block dead code for the adapter path. Please confirm which object MCore reads from - if it reads the adapter, these should be self.tokenizer / self.include_special_tokens. (Left inline.)

  2. (Low) include_special_tokens semantics. 'force_skip_special_tokens is False' yields include_special_tokens=True, but force_skip_special_tokens is None (the text-gen default) also yields True. Reasonable, but worth a one-line comment making the tri-state to boolean mapping explicit.

  3. (Low) Test coverage gap. The new test only covers force_skip_special_tokens=None (default). No coverage for the VLM path (force_skip_special_tokens=False via TokenizerWrapper) through the detokenizer, nor for the hasattr guard (a tokenizer already exposing .tokenizer / .include_special_tokens must not be overwritten).

Note: I could not read MCore HuggingFaceFastIncrementalDetokenizer source in this environment (3rdparty/Megatron-LM submodule not checked out), so finding 1 is a question, not a confirmed defect - please verify against the pinned MCore commit.

Suggested test cases

  • No perf tests impacted.
  • tests/unit_tests/inference/test_tokenizer.py::test_openai_streaming_accepts_hf_fast_tokenizer_adapter (added; run to confirm it passes)
  • Add: streaming detokenizer test with force_skip_special_tokens=False (VLM TokenizerWrapper path)
  • Add: hasattr guard test - a tokenizer already exposing .tokenizer / .include_special_tokens is left unmodified

Comment on lines +49 to +53
# MCore's incremental detokenizer treats ``_tokenizer`` as its Hugging Face wrapper.
if not hasattr(tokenizer, "tokenizer"):
tokenizer.tokenizer = tokenizer
if not hasattr(tokenizer, "include_special_tokens"):
tokenizer.include_special_tokens = force_skip_special_tokens is False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These attributes are set on tokenizer (the wrapped HF tokenizer, stored as self._tokenizer), but the object handed to HuggingFaceFastIncrementalDetokenizer in the new test is the adapter itself (self), not the wrapped tokenizer:

adapter = HFTokenizerAdapter(tokenizer)
detokenizer = HuggingFaceFastIncrementalDetokenizer(adapter, [])

If MCore's detokenizer reads .tokenizer / .include_special_tokens off the argument it's constructed with, it will look at adapter.tokenizer / adapter.include_special_tokens — which are never set here (the adapter has no __getattr__ forwarding). It would only "work" via a getattr(..., default) fallback in MCore, in which case this whole block is dead code w.r.t. the adapter path.

Can you confirm MCore reads these off the wrapped HF tokenizer and not off the adapter? If it reads off the adapter, these should be self.tokenizer = tokenizer and self.include_special_tokens = ....

@yaoyu-33
yaoyu-33 merged commit b4ca6ff into main Aug 22, 2026
90 checks passed
@yaoyu-33
yaoyu-33 deleted the yuya/verified-bug-hunter-openai-streaming-tokenizer-20260822 branch August 22, 2026 17:30

This branch was previously deployed

2 inactive deployments
public — 120e850b Deployed Aug 22, 2026 by copy-pr-bot[bot] via release / finalize / publish-docs #4896
test — 120e850b Deployed Aug 22, 2026 by copy-pr-bot[bot] via cicd-wait-in-queue #20873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant