Read Tool & Reasoning Tokens from GenAI API instead of Catalog Metadata - #838
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…uildToolCallContext
…o sayanshaw/fl-tool-tags
…with Fallback Map (#2215) # Add tool-calling and reasoning token IDs to GenAI Tokenizer ## Summary Adds four new token ID fields to the `model` section of `genai_config.json` — `bot_token_id`, `eot_token_id`, `bor_token_id`, `eor_token_id` — and exposes them on the **Tokenizer** (alongside `bos`, `eos`, `pad`), with a backward-compatible fallback for older model packages. This establishes a new naming convention for generation-level special tokens: | Abbreviation | Expansion | Purpose | |---|---|---| | **bot** | beginning of tool (call) | marks start of tool-call content | | **eot** | end of tool (call) | marks end of tool-call content | | **bor** | beginning of reasoning | marks start of reasoning/thinking content | | **eor** | end of reasoning | marks end of reasoning/thinking content | The naming mirrors `bos`/`eos`/`pad` — short, unambiguous, and follows the same access pattern (`GetBotTokenId()` alongside `GetBosTokenId()`). ## Changes ### Config parsing (`config.h` / `config.cpp`) - Added to `Config::Model`: `bot_token_id`, `eot_token_id`, `bor_token_id`, `eor_token_id` (int, default -1) - Parsed in `Model_Element::OnValue()` alongside existing token ID fields (bos, eos, pad) ### Tokenizer (`models/model.h` / `models/model.cpp`) - New private members: `int32_t bot_token_id_`, `eot_token_id_`, `bor_token_id_`, `eor_token_id_` - New public getters: `GetBotTokenId()`, `GetEotTokenId()`, `GetBorTokenId()`, `GetEorTokenId()` - Initialized in the Tokenizer constructor from config (identical pattern to bos/eos/pad) - **Fallback logic**: if any ID is -1 after reading config, attempts to resolve it by encoding known model-family-specific token strings through the vocabulary. This provides backward compatibility for Foundry Local consuming older model packages that predate these config fields. - Fallback map keyed by `model.type`: - `qwen2`/`qwen3`/`phi3` → `<tool_call>` / `</tool_call>` - `gptoss` → `<|start|>` / `<|call|>` - `qwen3` → `<think>` / `</think>` (reasoning) ### Public C API (`ort_genai_c.h` / `ort_genai_c.cpp`) - `OgaTokenizerGetBotTokenId(tokenizer, &token_id)` — returns BOT token id or -1 - `OgaTokenizerGetEotTokenId(tokenizer, &token_id)` — returns EOT token id or -1 - `OgaTokenizerGetBorTokenId(tokenizer, &token_id)` — returns BOR token id or -1 - `OgaTokenizerGetEorTokenId(tokenizer, &token_id)` — returns EOR token id or -1 Same pattern as `OgaTokenizerGetBosTokenId` / `OgaTokenizerGetPadTokenId`. ### C++ wrapper (`ort_genai.h`) - `int32_t OgaTokenizer::GetBotTokenId()` - `int32_t OgaTokenizer::GetEotTokenId()` - `int32_t OgaTokenizer::GetBorTokenId()` - `int32_t OgaTokenizer::GetEorTokenId()` ### C# (`Tokenizer.cs`) - `int Tokenizer.GetBotTokenId()` - `int Tokenizer.GetEotTokenId()` - `int Tokenizer.GetBorTokenId()` - `int Tokenizer.GetEorTokenId()` ### Java (`Tokenizer.java`) - `int tokenizer.getBotTokenId()` - `int tokenizer.getEotTokenId()` - `int tokenizer.getBorTokenId()` - `int tokenizer.getEorTokenId()` ### Objective-C (`ort_genai_objc.h`) - `- (int32_t)getBotTokenId:(NSError**)error` - `- (int32_t)getEotTokenId:(NSError**)error` - `- (int32_t)getBorTokenId:(NSError**)error` - `- (int32_t)getEorTokenId:(NSError**)error` ### Python (`python.cpp`) - `tokenizer.bot_token_id` (read-only property) - `tokenizer.eot_token_id` - `tokenizer.bor_token_id` - `tokenizer.eor_token_id` ### Tests (`test/c_api_tests.cpp`) - `TagId_Unknown` — verifies `gpt2` model (not in fallback map, no config IDs) returns -1 for all four - `TagId_FromConfig` — creates a temp model dir with token IDs in the `model` section, verifies correct parsing via the tokenizer ## genai_config.json format ```json { "model": { "type": "qwen3", "bos_token_id": 151643, "eos_token_id": 151645, "bot_token_id": 151657, "eot_token_id": 151658, "bor_token_id": 151659, "eor_token_id": 151660, "decoder": { ... } } } ``` All four fields are optional. If absent, the fallback map attempts to encode the known string via the tokenizer vocabulary. If the model type isn't in the fallback map either, -1 is returned (model doesn't support tool calling / reasoning). ## Motivation This is required for the Foundry Local Catalog v2 migration where tool/reasoning metadata is being removed from catalog. Also, the Foundry Local C++ SDK currently detects tool-call and reasoning tokens by **double-decoding every token** through both a normal and a special-token tokenizer stream, then doing `string.find("tool_call")` / `string.find("think")` per token. This is expensive. With token IDs exposed by GenAI on the Tokenizer, FL can do a single integer comparison per token in the decode loop — eliminating the special stream entirely for models with configured IDs. ## Design Decisions ### Why on the Tokenizer (not Model)? - BOS, EOS, and PAD token IDs are already accessed from the Tokenizer - Token IDs are a tokenizer-level concept — they describe the vocabulary - Follows existing pattern: `Tokenizer::GetBosTokenId()` → `Tokenizer::GetBotTokenId()` - No lazy init needed; resolved once in the constructor ### Why bot/eot/bor/eor naming? - Mirrors `bos`/`eos`/`pad` — short, memorable, consistent - Config field names also follow this pattern: `bot_token_id` alongside `bos_token_id` ### Fallback map - Exists specifically for Foundry Local backward compatibility with older model packages - Only triggered when config fields are not set - Resolved eagerly in the Tokenizer constructor (no lazy cache needed since tokenizer is already available) ## Related PRs - **Foundry-Local**: [Optimize tool/reasoning detection with token ID comparison in decode loop](microsoft/foundry-local#838) --------- Co-authored-by: Sayan Shaw <sayanshaw@microsoft.com>
…o sayanshaw/fl-tool-tags
There was a problem hiding this comment.
🟡 Changes recommended
The metadata data race and unresolved decode-path and behavior inconsistencies must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Migrates tool/reasoning marker metadata from the catalog to GenAI tokenizer APIs and adds token-ID-based marker detection.
Changes:
- Caches GenAI tag IDs and decoded strings.
- Enriches model metadata during loading.
- Adds ID-based marker detection with legacy fallback.
File summaries
| File | Review |
|---|---|
sdk_v2/cpp/src/model.h |
Makes stored model metadata mutable. No issue identified. |
sdk_v2/cpp/src/model.cc |
Critical (2 votes): Metadata mutation can race with unsynchronized readers. Moderate (2 votes): Support-flag behavior conflicts with the PR description. Nits (1 vote each): Add migration coverage; correct the qwen2 reasoning-marker example. |
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.h |
Defines cached tag metadata. No issue identified. |
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc |
Nit (1 vote): Add coverage for caching, partial/unsupported IDs, enrichment, and decode paths. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_generator.cc |
Moderate (2 votes): The implementation still decodes through both streams, so the claimed optimization is not realized. |
Review details
Suppressed comments (3)
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc:104
- No test covers this new tag cache or its consumers. Existing C++ chat/model tests provide coverage for these areas, but none verifies configured versus unsupported/partial tag IDs, preservation of catalog values during enrichment, or marker emission through the fast and fallback decode paths. Add focused coverage before merging this metadata migration.
const GenAIModelInstance::TagInfo& GenAIModelInstance::GetTagInfo() {
std::call_once(tag_info_init_flag_, [this]() {
sdk_v2/cpp/src/model.cc:524
- The central Catalog v2 behavior has no test asserting that a load populates missing tool/reasoning marker properties while preserving catalog-provided values. Existing model tests exercise
Load()and metadata access, so add coverage for both cases to prevent this migration from silently regressing.
const auto& tag_info = result.model->GetTagInfo();
sdk_v2/cpp/src/model.cc:540
- The fallback-map example is inaccurate: qwen2 has bot/eot fallback IDs, while bor/eor fallbacks are defined for qwen3. Please avoid citing qwen2 as a family with reasoning markers.
// from the mere presence of tag token IDs. The GenAI fallback map defines bor/eor
// tokens for entire model families (e.g. all qwen2), but that doesn't mean every
// variant actually supports reasoning (qwen2.5-0.5b is not a thinking model).
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
Thanks for this - the overall approach is clean and I like that it mirrors the existing EOS token pattern with call_once caching, keeps the slow path for backward compatibility, and deliberately avoids guessing the tool/reasoning support flags from the family-wide fallback map. Nice.
I left a few inline comments. The main one worth resolving before merge is the thread-safety of making ModelInfo writable and updating it inside Load(). The rest are smaller: the performance description doesn't quite match what the code does, a couple of description/code mismatches, and a small formatting nit.
A few non-blocking notes:
- Dependency: this needs the newer onnxruntime-genai package that has
GetBotTokenId/GetEotTokenId/GetBorTokenId/GetEorTokenId.deps_versions.jsonisn't bumped in this PR, so CI won't build until that lands. Worth calling out in the PR so it isn't merged early. - Tests: none here (understandable, since it needs the new package). Once the package is available, a small test that a model with these tokens fills in the tag strings, and a model without them still works via the old path, would give good coverage.
- Description mismatch: the "Changes -> Model::Load()" section says it "Infers supports_tool_calling / supports_reasoning from tag ID presence". The code intentionally does NOT do this (and the code comment explains why - good call). Please update the description so it matches.
Thanks for the detailed review. Addressed the comments in the latest update:
The changes are pushed in commit d55c5ee (Avoid mutating published model metadata). |
Migrate tool/reasoning metadata from catalog to GenAI + optimize decode loop
Summary
Two goals:
Catalog v2 migration: Tool-call and reasoning token metadata (
toolCallStart,toolCallEnd,reasoningStart,reasoningEnd) is being removed from the catalog. This PR sources it from GenAI'sgenai_config.jsoninstead, via the newOgaTokenizer::GetBotTokenId()/GetEotTokenId()/GetBorTokenId()/GetEorTokenId()APIs, and writes the decoded strings intoModelInfoso all existing downstream consumers continue working.Decode loop optimization: The current per-token detection of special tokens (double-decode +
string.find()) is replaced with a single integer comparison against cached token IDs - the same pattern used for EOS detection.Naming Convention
This PR adopts the new bot/eot/bor/eor naming convention introduced in this GenAI PR:
Mirrors
bos/eos/padin both naming and access pattern (same Tokenizer class).Performance
Before (per token): 2 tokenizer stream decodes + 2
string.find()+ string comparisonAfter (per token): 1 integer comparison + 1 normal-stream decode
The updated path eliminates the special tokenizer stream from decode and keeps only the normal stream.
There is no runtime double-decode fallback in
Decode().Storage (follows EOS pattern)
eos_token_ids_GenAIModelInstanceDecode()- filters EOS tokenstag_info_(IDs + strings)GenAIModelInstanceDecode()- fast-path detectionModelInfo.string_propertiesBuildToolCallContext()->ToolCallStreamAccumulatorChanges
GenAIModelInstance(genai_model_instance.h/.cc)TagInfostruct withbot_id/eot_id/bor_id/eor_id+ decoded stringsGetTagInfo()- lazy-cached viastd::call_once(same pattern asGetEosTokenIds())tokenizer_->GetBotTokenId()etc. (4 getter calls, one-time)tokenizer_with_special_->Decode(&id, 1)to get the string (4 decodes, one-time)OnnxChatGenerator::Decode()(onnx_chat_generator.cc)Model::Load()(model.cc)ModelInfostring properties fromGetTagInfo()cached stringssupports_tool_calling/supports_reasoningfrom tag ID presenceNo changes to
ToolCallContext,ToolCallStreamAccumulator, orBuildToolCallContext()ModelInfoas before - single source of truthDependency
Requires updated
onnxruntime-genainuget withOgaTokenizerGetBotTokenId/GetEotTokenId/GetBorTokenId/GetEorTokenIdAPIs.How it works
Before (per token, every single generated token):
Cost per token: 2 tokenizer stream decodes + 2
string.find()+ string comparison - on every token, even though markers appear maybe 2-4 times per generation.After (per token):
Cost per token: 1 integer comparison + 1 normal decode. No special stream. No string search.
Estimated performance gain
For a typical 500-token generation:
~50% reduction in tokenizer decode calls and complete elimination of string search operations in the hot loop. The special tokenizer stream is never even created/maintained for models with tag IDs configured.
Tag names supported by the GenAI API
"tool_call_start","tool_call_end"- tool call delimiters"reasoning_start","reasoning_end"- chain-of-thought reasoning delimitersTesting
Validated with local build and focused integration tests on the updated branch:
foundry_local_teststarget)WebServiceIntegrationTest.ChatCompletionsWithResponseFormat)The hot decode path now runs single-stream decode for all tokens.