Skip to content

[sgl-router] Allow load-only routing without a tokenizer - #41611

Merged
ShangmingCai merged 4 commits into
mainfrom
codex/pd-b-optional-tokenizer
Oct 1, 2026
Merged

ShangmingCai merged 4 commits into
mainfrom
codex/pd-b-optional-tokenizer

Conversation

@sherlockwu

@sherlockwu sherlockwu commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Load-only routing does not need a tokenizer, but the router always downloads and loads one at startup.

Changes

  • --no-tokenizer skips tokenizer loading. It cannot be combined with --tokenizer-path.
  • Startup rejects --no-tokenizer when the built routing reads request tokens (cache-aware routing, prefix-cache fuse terms or filters, length-bounded reorg buckets).
  • /v1/tokenize and /v1/detokenize return 400 tokenizer disabled for the served model instead of 404.

CI States

Latest PR Test (Base): ✅ Run #36826694042
Latest PR Test (Extra): ❌ Run #36826693833
Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 29, 2026
@sherlockwu
sherlockwu added this pull request to stack #41614 September 29, 2026 01:49
@sherlockwu
sherlockwu force-pushed the codex/pd-b-optional-tokenizer branch 3 times, most recently from 0144054 to 9f96e38 Compare September 29, 2026 05:25
@sherlockwu sherlockwu added router run-ci CI: run the baseline test suite on this PR labels Sep 29, 2026

/// Cache matching and length-based buckets require a real tokenizer.
/// Load-only routing can use the request-size estimate for accounting.
pub fn requires_tokenizer(&self) -> bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: requires_tokenizer() mirrors Policy::needs_request_tokens() by hand. It matches today, but if a future token-dependent term or filter is added and this list isn't updated, PrefixCachePolicy-style terms silently return flat scores instead of failing at startup. Consider validating against the built policy (needs_request_tokens()) instead, or add a test that asserts the two agree for every PolicyKind/term/filter.

@sherlockwu sherlockwu Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I removed requires_tokenizer().

pub model_id: String,

/// Local tokenizer.json or HuggingFace repo id. Defaults to --model-id; honors HF_TOKEN / HF_HOME.
/// "none" skips it for load-only policies; cache-aware and length-bucket routing require one.

@whybeyoung whybeyoung Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Consider a dedicated --no-tokenizer flag instead of the magic "none" value, which could collide with a real path and doesn't match None/NONE.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I replaced the "none" value with a dedicated --no-tokenizer flag (conflicts_with = "tokenizer_path"). Internally ModelConfig.tokenizer_path is now Option<String>, so there's no sentinel value left.

@github-actions github-actions Bot added the npu label Oct 1, 2026
@sherlockwu
sherlockwu force-pushed the codex/pd-b-optional-tokenizer branch from 3a8dc45 to f979c83 Compare October 1, 2026 01:53
@sherlockwu
sherlockwu requested a review from whybeyoung October 1, 2026 02:06
Base automatically changed from codex/pd-a-bootstrap-readiness to main October 1, 2026 06:32
Kan Wu and others added 3 commits October 1, 2026 14:32
`--tokenizer-path none` skips tokenizer loading for load-only policies on
both routing paths. Cache-aware routing, prefix-cache terms or filters, and
--bucket-config still require a tokenizer and fail validation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dedicated flag cannot collide with a real path; ModelConfig now carries
`tokenizer_path: Option<String>`.

Startup now rejects --no-tokenizer by asking the built routing whether it
reads request tokens, instead of mirroring each policy in the config. Reorg
policies gain `needs_request_tokens`, so the check covers both routing paths
and length-bounded reorg buckets. --bucket-config is still rejected during
config validation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ShangmingCai
ShangmingCai force-pushed the codex/pd-b-optional-tokenizer branch from f979c83 to 46e169a Compare October 1, 2026 06:32
The reorg handler passed a literal `true`, so `Bucket::needs_request_tokens`
only ever ran at startup to vet `--no-tokenizer`; at request time every reorg
request tokenized regardless of what its buckets select on. Startup and
dispatch could therefore disagree: the guard admits a load-only bucket set
without a tokenizer, and the handler then asks for tokens anyway.

Pass the resolver's own answer, which is what the old comment described.
Behaviour is unchanged wherever tokens are read -- a bounded bucket, a
prefix-matching group policy, or any worker-bound forwarding, which already
forces tokenization through `can_forward_input_ids`. What it removes is a
`serde_json::from_slice` of every body on the load-only path, whose result
`build_outgoing_body` discards outside PD mode (`needs_parse` is false, so it
returns `body.clone()`).

Cover the predicate's length arms directly, since they now gate request-time
tokenization: the existing tests only vary the group policy, leaving
`limits.min`, `limits.max` and `max_context_tokens` untested.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ShangmingCai
ShangmingCai merged commit 4663796 into main Oct 1, 2026
93 of 97 checks passed
@ShangmingCai
ShangmingCai deleted the codex/pd-b-optional-tokenizer branch October 1, 2026 10:30
ShangmingCai added a commit that referenced this pull request Oct 1, 2026
Resolves ten conflicts left by #41610, #41611 and #41612 landing on main while
this branch still carried their pre-squash commits. Two needed judgement rather
than a side:

- `readyz`: main gained the peer-bootstrap seed gate (#40699) while this branch
  rewrote the pool check for version groups. The hunks overlap, and taking
  either side whole drops the other, so the resolution keeps this branch's
  group-aware `pool_ready` *and* main's `&& ctx.kv_bootstrap_admit_ready()`.
- `chat/forward.rs`: this branch carries #41612's pre-fix copy; main has the
  post-fix one. The version-group feature does not touch this file at all, so
  it takes main's version, keeping the `cancelled` outcome #41612 records for a
  decode dispatch that prefill's failure abandoned.
- `pd_bootstrap_injection/reliability.rs` (add/add): keeps this branch's
  `..Default::default()` spec and main's two #41612 assertions.

The rest are the mechanical `..Default::default()` churn this branch introduced
meeting fields main spells out, plus a `readiness_rejects_portless_prefill` body
git aligned against its own merged copy; resolved so each test and helper is
defined once.

Verified on the merge result: 1017 lib + 129 component + 157 proxy tests pass,
clippy -D warnings and rustfmt clean. The two fixes a careless resolution would
have reverted are covered by tests that still pass -- the five
`pd_bootstrap_injection::reliability` cases and the four seed-gate `readyz`
cases.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation npu router run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants