[GGUF] Use the OpenVINO GGUF frontend: tokenizer from rt_info, stateful graph - #4318
[GGUF] Use the OpenVINO GGUF frontend: tokenizer from rt_info, stateful graph#4318mvafin wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches GenAI’s GGUF path to use the OpenVINO GGUF frontend for model conversion, builds tokenizers from frontend-provided rt_info metadata (avoiding re-reading the .gguf), and adds WWB + sample coverage around GGUF correctness/performance.
Changes:
- Route
.ggufconversion through the OpenVINO GGUF frontend, applyMakeStatefulvia a scopedDecoderTransformationExtension, then runAdaptToGenAI. - Build GGUF tokenizer/detokenizer from converted-model
rt_info, with updated BOS/EOS handling viaCombineSegmentsacross tokenizer flavors. - Add WWB opt-in GGUF accuracy tests (GenAI vs llama.cpp) and new GGUF-focused C++ samples/benchmarks.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/who_what_benchmark/whowhatbench/wwb.py | Graceful tokenizer loading fallback for GenAI+GGUF; align llama.cpp generation signature with other backends. |
| tools/who_what_benchmark/whowhatbench/model_loaders.py | Add GGUF-aware GenAI wrapper behavior; increase llama.cpp default context window. |
| tools/who_what_benchmark/tests/test_cli_text_gguf.py | New opt-in heavy GGUF accuracy suite comparing GenAI vs llama.cpp across architectures. |
| tools/who_what_benchmark/requirements.txt | Add llama-cpp-python dependency for the llama.cpp reference path. |
| src/cpp/src/tokenizer/tokenizer.cpp | Add Tokenizer ctor that builds from a GGUF-frontend ov::Model. |
| src/cpp/src/tokenizer/tokenizer_impl.hpp | Declare GGUF-from-model tokenizer path and shared finalization helper. |
| src/cpp/src/tokenizer/tokenizer_impl.cpp | Implement GGUF-from-model tokenizer creation; factor common GGUF tokenizer finalization and shared-object resolution. |
| src/cpp/src/llm/pipeline.cpp | Default GGUF to SDPA backend and build tokenizer from model rt_info for GGUF. |
| src/cpp/src/gguf_utils/gguf.hpp | Add metadata-only GGUF read API declaration. |
| src/cpp/src/gguf_utils/gguf.cpp | Implement get_gguf_metadata() to load only KV metadata without tensor reads/dequant. |
| src/cpp/src/gguf_utils/gguf_tokenizer.hpp | Add tokenizer creation API from GGUF-frontend model rt_info. |
| src/cpp/src/gguf_utils/gguf_tokenizer.cpp | Major tokenizer builder upgrade: SentencePiece proto wiring, gemma4/plamo2 handling, explicit BOS/EOS segments, rt_info config conversion. |
| src/cpp/src/gguf_utils/gguf_modeling.cpp | Replace GenAI GGUF reader with OpenVINO GGUF frontend conversion + scoped MakeStateful + AdaptToGenAI. |
| src/cpp/include/openvino/genai/tokenizer.hpp | Public API: Tokenizer ctor from GGUF-frontend model. |
| src/cpp/CMakeLists.txt | Link against OpenVINO GGUF frontend target (gguf/ggml) when ENABLE_GGUF=ON. |
| samples/cpp/text_generation/gguf_save_ir.cpp | New sample to convert GGUF through public pipeline and save IR. |
| samples/cpp/text_generation/gguf_arch_check.cpp | New per-arch GGUF correctness/perf/memory check utility. |
| samples/cpp/text_generation/CMakeLists.txt | Register new GGUF samples/bench. |
| samples/cpp/text_generation/bench_gguf_perf.cpp | New minimal GGUF perf benchmark (TTFT/TPOT/throughput) with PA-prefix-cache control. |
| samples/cpp/chat/CMakeLists.txt | Add unified chat sample build (downloads stb header like other samples). |
| samples/cpp/chat/chat.cpp | New unified text+VLM chat sample supporting .gguf and IR directories. |
| samples/CMakeLists.txt | Add new cpp/chat samples subdirectory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dcf4f30 to
1ea1f55
Compare
1ea1f55 to
6750c5e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/cpp/src/llm/pipeline.cpp:319
- When building a Tokenizer from an already-read GGUF model, TokenizerImpl gets enable_save_ov_model but has no save_dir, so openvino_tokenizer.xml/openvino_detokenizer.xml are no longer serialized even when ov::genai::enable_save_ov_model=true. This is a behavioral regression vs the from-path GGUF tokenizer construction.
// For GGUF the frontend attaches the tokenizer metadata to the model's rt_info, so build the
// tokenizer from the already-read model instead of re-opening the .gguf.
const Tokenizer tokenizer = (models_path.extension() == ".gguf")
? Tokenizer(model, properties)
: Tokenizer(models_path, properties);
tools/who_what_benchmark/requirements.txt:37
- llama-cpp-python is only needed for the opt-in GGUF accuracy tests (gated by WWB_GGUF_TESTS=1), but adding it unconditionally to requirements.txt forces it to be installed for all WWB users/platforms and can fail on unsupported platforms.
# llama.cpp reference oracle for the opt-in GGUF accuracy tests (test_cli_text_gguf.py, --llamacpp)
llama-cpp-python>=0.3.0
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:6
- gguf_tokenizer.cpp now uses std::none_of (and std::memcpy throughout), but it doesn’t include (or ). Relying on transitive includes is non-portable and can break builds on some toolchains.
#include <limits>
#include <cstdint>
#include <set>
samples/cpp/chat/chat.cpp:55
- The comment above ov::cache_dir is self-contradictory (“Skip for GGUF paths … re-runs benefit too”), but the code always enables caching. This is confusing for sample users.
// Compiled model cache: saves re-compilation time on subsequent runs.
// Skip for GGUF paths since the model is built on-the-fly and the cache
// key tracks the IR blob — re-runs benefit too.
props[ov::cache_dir.name()] = "ov_model_cache";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tools/who_what_benchmark/requirements.txt:37
llama-cpp-pythonbecomes a mandatorywhowhatbenchinstall dependency (setup.py reads requirements.txt into install_requires), but it’s only needed for opt-in GGUF reference runs. This can significantly increase install time and may fail on systems without a C++ toolchain.
# llama.cpp reference oracle for the opt-in GGUF accuracy tests (test_cli_text_gguf.py, --llamacpp)
llama-cpp-python>=0.3.0
tools/who_what_benchmark/tests/test_cli_text_gguf.py:118
- The
xfailreason claims lack of gemma4 tokenizer model support, but the implementation in this PR addsgemma4handling. If gemma4 is still expected to fail, the reason should reflect the actual blocker (e.g., missing tokenizer ops registration).
reason="GenAI gguf tokenizer lacks 'gemma4' model support (gguf_tokenizer.cpp); "
"graph conversion + KV-cache fix work, only the tokenizer blocks end-to-end.",
samples/cpp/chat/chat.cpp:54
- The comment says to skip the compiled-model cache for GGUF paths, but the code always enables
ov::cache_dirregardless of model type.
// Compiled model cache: saves re-compilation time on subsequent runs.
// Skip for GGUF paths since the model is built on-the-fly and the cache
// key tracks the IR blob — re-runs benefit too.
tools/who_what_benchmark/tests/test_cli_text_gguf.py:101
- These comments state that
gguf_tokenizer.cpprejectstokenizer.ggml.model='gemma4', but this PR adds explicitgemma4support ingguf_tokenizer.cpp. The test comment is now misleading.
This issue also appears on line 117 of the same file.
# Gemma family: the frontend converts the graph fine (and the gemma4 KV-cache-precision fix
# applies), but GenAI's GGUF *tokenizer* loader currently supports only the 'gpt2' and
# 'llama' tokenizer models -- gemma/gemma2 ('llama' SPM) load, but gemma4 stores
# tokenizer.ggml.model='gemma4' (a BPE variant) which gguf_tokenizer.cpp rejects, so the
# LLMPipeline can't be built end-to-end yet. xfail until GenAI adds the gemma4 tokenizer.
- New create_tokenizer_from_model() builds the OV tokenizer/detokenizer from the GGUF tokenizer metadata the frontend attached to the model's rt_info, so the .gguf is not re-opened. New Tokenizer(model, props) ctor + TokenizerImpl path, shared finalize_gguf_tokenizer() tail. - LLMPipeline routes GGUF tokenizer construction through the already-read model (single .gguf read instead of two). - gemma4 tokenizer: SPM-style BPE (metaspace normalize, newline-only split, raw-UTF8 BBPE) in the GGUF tokenizer builder. - Link the OpenVINO GGUF frontend (openvino::frontend::ggml) for the AdaptToGenAI path; bump openvino_tokenizers submodule to add RegexNormalization to the op factory. - WWB: GGUF accuracy tests vs llama.cpp (test_cli_text_gguf.py) + loader fixes.
The OpenVINO GGUF frontend target was renamed from openvino::frontend::ggml to openvino::frontend::gguf. Prefer the new name when linking the GGUF conversion path (create_from_gguf + AdaptToGenAI), falling back to the legacy ggml target for older OpenVINO builds. Also register the bench_gguf_perf and gguf_save_ir text-generation samples.
gguf_arch_check runs one .gguf through LLMPipeline and reports correctness, performance and peak memory in a single process: greedy generation on a factual prompt (so the answer is checkable), TTFT/TPOT, and peak anonymous vs file-backed resident pages sampled from /proc/self/smaps_rollup. This is the harness behind the per-architecture status tables in the GGUF frontend docs. bench_gguf_perf.cpp and gguf_save_ir.cpp were already listed in SAMPLE_LIST but their sources had never been committed, so configuring the samples failed. Add them here as well.
ATTENTION_BACKEND=PA routes through the ContinuousBatching adapter, and get_latency_oriented_scheduler_config() enables prefix caching by default. The sample repeats one fixed prompt for `iters` iterations to amortize the first-request dynamic-shape compile, so every iteration after the first was a cache hit: the reported TTFT stopped being prefill work. On Llama-3.2-1B that reads 125 ms cached vs 300 ms uncached, while SDPA measures 304 ms -- making PA look 2.4x faster at prefill than an identical computation. The asymmetry is what makes it a comparison hazard: SDPA ignores this knob, and neither llama.cpp reference path caches across runs (llama-bench clears the cache inside its rep loop before the timer; llama-cli is single-shot). So one cached number was being compared against two uncached ones. Pass an explicit SchedulerConfig with prefix caching off, otherwise matching the latency-oriented default, so prefill is measured every iteration. Prefix caching remains a real PA capability -- it is just not prefill throughput, and belongs in a separate measurement. Also drop the now-stale "the GGUF-frontend graph is SDPA-only" comment: the converted graph is valid under both SDPA and PagedAttention.
The GGUF frontend is universal and always converts to a stateless graph, every KV cache an explicit input/output pair, exactly as optimum-intel exports before it applies its own make-stateful transformation. Statefulness is the consumer's choice, so make it here: register ov::frontend::gguf::pass::MakeStateful as a DecoderTransformationExtension and the frontend runs it in its normalization stage, ahead of the built-in stateless lowering, so each cache becomes a ReadValue/Concat/Assign state instead. Drive the frontend directly rather than through core.read_model(model_path) so the extension is scoped to this one conversion -- ov::Core::add_extension would be global. AdaptToGenAI then does what it always did: the IO contract.
Two independent defects, both of which made encode() disagree with llama.cpp:
* The BPE paths (gpt2, gemma4) never read tokenizer.ggml.add_bos_token at
all. add_bos is an attribute of SentencepieceTokenizer, so the SPM path
got it for free, but BPETokenizer has no such knob and the branch went
straight from parse_bbpe_config to truncation. BOS-sensitive models were
silently fed a BOS-less prompt: muse-glimmer (gpt2 + add_bos_token=true)
picked " The" instead of " It" after "The capital of France is" and
degenerated into a loop; llama3 and mistral3 lost their BOS the same way.
* add_special_tokens=false was a no-op on BOTH paths.
ov::genai::MakeAddSpecialTokensSatateful implements the runtime flag by
finding a CombineSegments node and wrapping each special-token segment's
`ends` in a Select. The GGUF tokenizer built no CombineSegments, so the
pass bailed out and the flag did nothing: SPM always prepended BOS, BPE
never did.
Fix both at once by emitting BOS/EOS as an explicit CombineSegments segment
for every flavour, and asking SentencepieceTokenizer for add_bos/add_eos=false
so the SPM path goes through the same mechanism instead of baking the decision
into a compile-time attribute.
Two placement constraints are load-bearing:
* The node goes AFTER the truncation block. MakeAddSpecialTokensSatateful
tells the main sequence from a special-token segment by its begins being a
Subtract (and its ends an Add/Truncate); emitting before the truncation
makes the sequence look like a special-token segment and breaks the toggle.
* When the GGUF omits add_bos_token the default has to follow llama.cpp
(llama-vocab.cpp): SPM defaults true, BPE defaults false EXCEPT the
LLAMA_VOCAB_PRE_TYPE_LLAMA3 pre-tokenizer group, tekken and chameleon,
plus gemma4 which is forced true. Llama-3.2 is exactly this case --
pre=llama-bpe with the key absent -- and llama.cpp does add its BOS.
Verified on CPU against llama.cpp: muse-glimmer now reproduces the reference
completion token-for-token ("Paris. It is the largest city in France and is
known for its rich history, culture, and art. ..."), and Llama-3.2-1B's prompt
tokenizes to the same 6 ids llama-eval-callback reports. Checked with specials
on and off: muse-glimmer, Llama-3.2-1B, Ministral-3-3B (tekken), gemma-4-E2B,
Qwen3-0.6B (add_bos_token=false, correctly unchanged), and on the SPM side
gemma-3-1b, gemma-2b and mistral-7b, whose token ids are unchanged with
specials on and now drop BOS when asked. End-to-end generation re-checked on
llama, qwen3, gemma3 and mistral3 with no regressions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/cpp/src/tokenizer/tokenizer_impl.cpp:272
- In the GGUF-from-model tokenizer constructor, the
ov::genai::enable_save_ov_modelproperty is extracted but can never take effect becausesave_diris passed as an empty path. This means a user can request serialization and it will be silently ignored. Prefer failing fast with a clear message (or explicitly disable saving) to avoid surprising behavior.
auto [filtered_properties, enable_save_ov_model] = utils::extract_gguf_properties(properties);
std::shared_ptr<ov::Model> ov_tokenizer, ov_detokenizer;
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:8
- This file uses
std::string_view(e.g.,std::string_view::npos) andstd::memcpy, but it doesn't include the corresponding standard headers directly. Elsewhere in the codebase these are included explicitly (e.g.,src/cpp/src/logger.cpp:16includes<string_view>,src/cpp/src/gguf_utils/gguf.cpp:7includes<cstring>). Adding the missing includes here avoids relying on transitive includes.
#include <algorithm>
#include <limits>
#include <cstdint>
#include <set>
tools/gguf_arch_check/gguf_arch_check.cpp:23
gguf_arch_check.cppusesstd::stringbut doesn't include<string>. Relying on indirect inclusion from other headers is brittle and can break builds depending on the standard library implementation / include order.
#include <atomic>
#include <chrono>
#include <fstream>
#include <iomanip>
#include <iostream>
…tokenizer nits
Samples: the text-generation README already documents that any sample takes a .gguf path
directly, and the pipeline change in this PR is what makes that true, so the new samples were
redundant with what already exists:
* samples/cpp/chat -- a general LLM+VLM chat program, not GGUF-specific, and duplicating
chat_sample.cpp and visual_language_chat.cpp.
* bench_gguf_perf -- benchmark_genai already reports TTFT/TPOT and already disables prefix
caching with the same scheduler settings.
* gguf_save_ir -- a three-line wrapper around the existing enable_save_ov_model property.
All three are dropped; samples/ is now untouched by this PR. gguf_arch_check is kept but moved
to tools/gguf_arch_check: it is a triage harness (it samples /proc/self/smaps_rollup to produce
the per-architecture memory table that drives CI runner sizing), not an example of API usage.
It gains a README, since the samples were shipping undocumented.
Review fixes:
* Include <algorithm> in gguf_tokenizer.cpp: it uses std::none_of and was relying on a
transitive include.
* enable_save_ov_model was silently a no-op for .gguf. The tokenizer is now built from the
model's rt_info, and that path has no source directory to serialize the tokenizer IRs into,
so finalize_gguf_tokenizer skipped writing them. Use the from-file tokenizer path when
saving is explicitly requested.
* Move llama-cpp-python out of WWB's install_requires into a whowhatbench[gguf] extra. The
GGUF accuracy tests are opt-in (WWB_GGUF_TESTS=1), and it builds llama.cpp from source, so
every WWB install was paying for a toolchain it does not otherwise need.
* Accept a bare int in read_tokenizer_flag/read_tokenizer_id. Both GGUF readers store scalars
as shape-{} tensors, so this is not reachable today, but the readers silently returned the
default (dropping BOS/EOS ids) for any other representation.
* Correct the gemma4 xfail reason: gemma4 tokenizer support is in this PR; what remains is the
openvino_tokenizers op-factory dependency.
Reformat the changed Python lines with darker/ruff to satisfy the lint job.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:8
gguf_tokenizer.cppusesstd::memcpyin multiple new code paths but does not include<cstring>. Relying on transitive includes is non-portable and can break compilation with stricter toolchains.
#include <algorithm>
#include <limits>
#include <cstdint>
#include <set>
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:683
parse_spm_config()accessestokenizer_configusing.at("tokens")/.at("token_type")/.at("scores"). If any key is missing (or has an unexpected variant alternative), this throwsstd::out_of_rangebeforeOPENVINO_ASSERTcan produce a clear error message.
if (auto val = std::get_if<std::vector<std::string>>(&tokenizer_config.at("tokens")))
vocab = *val;
if (auto val = std::get_if<ov::Tensor>(&tokenizer_config.at("token_type")))
token_types_tensor = *val;
tools/gguf_arch_check/README.md:28
- README usage string is out of sync with the tool:
gguf_arch_check.cppaccepts an optional 4th argument to controlapply_chat_template, but the README documents only 3 arguments.
```sh
gguf_arch_check <model.gguf> <max_new_tokens> <perf_iters>
**tools/gguf_arch_check/gguf_arch_check.cpp:73**
* The tool actually accepts an optional 4th CLI argument (`apply_chat_template`), but the runtime usage string printed on invalid args does not mention it. This makes it harder to discover the flag.
if (argc < 4) {
std::cerr << "Usage: " << argv[0] << " <model.gguf> <max_new_tokens> <perf_iters>\n";
return 1;
**tools/who_what_benchmark/tests/test_cli_text_gguf.py:104**
* `GGUF_MODELS` is described as containing “small” representative GGUFs, but the `gpt-oss-20b` case is likely to be extremely large and slow even when the opt-in suite is enabled, risking timeouts/runner OOM. Consider gating this specific model behind an additional env var so the rest of the GGUF coverage remains usable.
pytest.param(
"gpt-oss",
"ggml-org/gpt-oss-20b-GGUF",
"gpt-oss-20b-mxfp4.gguf",
id="gpt-oss-20b",
</details>
Two fixes found by comparing the frontend against the pre-frontend reader on the same binary and hardware. PagedAttention: the frontend's graph is not accepted by SDPAToPagedAttention, and pushing it through the continuous-batching adapter failed inside the plugin with a shape mismatch on PagedAttentionExtension. Defaulting .gguf to SDPA was not enough, because merely passing a scheduler_config makes explicitly_requires_paged_attention() true and that selects the CB adapter whatever the backend is -- so `benchmark_genai -m model.gguf`, which sets one only for latency tuning, crashed. Route GGUF to the stateful path before those branches, say so in the log when a PA configuration is dropped, and strip scheduler_config, which the plugin otherwise rejects as an unknown property. The legacy reader's graph does convert to PagedAttention, so it is left alone. OPENVINO_GENAI_GGUF_LEGACY_READER=1 selects the old reader. It is a temporary fallback until that code is deleted, and it is not equivalent: it only ever handled llama / qwen2 / qwen3, it fails outright on some files that the frontend reads (Qwen2.5-0.5B-Instruct-Q4_K_M throws "gguf_tensor_to_f16 failed"), and it never looks at rope_freqs.weight, so llama-3 RoPE scaling is silently not applied and its output differs from the frontend's. Because the model it returns carries no frontend rt_info, the tokenizer falls back to being built from the .gguf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:996
- In the SentencePiece detokenizer branch, spm_scores can remain empty (and tokenizer_config.at("scores") can throw) but build_spm_model_proto() unconditionally indexes scores[i] for every token, which would be out-of-bounds and crash. This should fail fast with an OPENVINO_ASSERT that scores are present and match the vocab size (as the tokenizer path already does).
std::vector<float> spm_scores;
if (auto val = std::get_if<ov::Tensor>(&tokenizer_config.at("scores"))) {
const ov::Tensor& st = *val;
spm_scores.assign(st.data<float>(), st.data<float>() + st.get_size());
}
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:683
- parse_spm_config() indexes tokenizer_config with .at("tokens")/.at("token_type")/.at("scores"). If any key is missing (or has an unexpected type), this will throw std::out_of_range before the OPENVINO_ASSERT checks below, producing a much less actionable error than the intended "[gguf tokenizer] SentencePiece: ..." messages.
This issue also appears on line 992 of the same file.
if (auto val = std::get_if<std::vector<std::string>>(&tokenizer_config.at("tokens")))
vocab = *val;
if (auto val = std::get_if<ov::Tensor>(&tokenizer_config.at("token_type")))
token_types_tensor = *val;
| auto mask_i32 = std::make_shared<v0::Convert>(mask, element::i32); | ||
| auto ax0_1d_i32 = std::make_shared<v0::Constant>(element::i32, Shape{1}, std::vector<int32_t>{0}); | ||
| auto row_splits = std::make_shared<v1::ReduceSum>(mask_i32, ax0_1d_i32, false)->output(0); | ||
|
|
||
| // begins = row_splits[0:B], ends = row_splits[1:B+1] | ||
| auto one_i32 = std::make_shared<v0::Constant>(element::i32, Shape{1}, std::vector<int32_t>{1}); | ||
| auto zero_i32 = std::make_shared<v0::Constant>(element::i32, Shape{1}, std::vector<int32_t>{0}); | ||
| auto int32_max = std::make_shared<v0::Constant>(element::i32, Shape{1}, | ||
| std::vector<int32_t>{std::numeric_limits<int32_t>::max()}); | ||
| // B as a 1D i32 tensor (v8::Slice stop must be 1D) | ||
| auto B_i32_1d = std::make_shared<v0::Unsqueeze>( | ||
| std::make_shared<v0::Convert>(B, element::i32), | ||
| std::make_shared<v0::Constant>(element::i64, Shape{1}, std::vector<int64_t>{0})); | ||
| auto axes_0_i32 = std::make_shared<v0::Constant>(element::i32, Shape{1}, std::vector<int32_t>{0}); | ||
| auto begins = std::make_shared<v8::Slice>(row_splits, zero_i32, B_i32_1d, one_i32, axes_0_i32)->output(0); | ||
| auto ends_node = std::make_shared<v8::Slice>(row_splits, one_i32, int32_max, one_i32, axes_0_i32)->output(0); |
…tokenizer saves The WWB GGUF suite compares GenAI loading a .gguf through the OpenVINO frontend against llama.cpp running the same file, so a conversion regression shows up as a similarity drop instead of plausible-looking but wrong text. It has never run anywhere: the whole file self-skips unless WWB_GGUF_TESTS=1 and nothing sets that, in CI or otherwise. So the suite exists but guards nothing. Turn it on for the subset that fits a runner. The eleven fixtures span 551 MB to 11.5 GB, so mark the four sub-1 GB ones (llama / qwen2 / qwen3 / hunyuan-dense, ~2.4 GB together) `gguf_small` and add a "WWB tests (GGUF)" job that runs `-m gguf_small`. The larger architecture fixtures stay an opt-in local run. The job name has to start with "WWB": that is what gates the existing llama-cpp-python install step, and llama.cpp is the reference side here. Separately, Tokenizer(ov::Model, properties) accepts enable_save_ov_model and then cannot honour it -- it has no source directory, so finalize_gguf_tokenizer is handed an empty save_dir and writes nothing. LLMPipeline already dodges this by routing to the from-file constructor whenever saving was asked for, but a direct caller of the Tokenizer API gets no openvino_tokenizer.xml, no openvino_detokenizer.xml and no explanation. Say so instead of failing silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/cpp/src/gguf_utils/gguf_tokenizer.cpp:615
- In sparse_to_ragged(), v1::ReduceSum and v8::Slice are given i32 axis / begin/end/stride constants. Elsewhere in the codebase these ops are consistently driven by i64 constants, and some OpenVINO validations expect integral i64 for axes/index tensors. Using i32 here risks a runtime validation error when building the tokenizer model.
auto ax0_1d_i32 = std::make_shared<v0::Constant>(element::i32, Shape{1}, std::vector<int32_t>{0});
auto row_splits = std::make_shared<v1::ReduceSum>(mask_i32, ax0_1d_i32, false)->output(0);
src/cpp/src/tokenizer/tokenizer_impl.cpp:280
- TokenizerImpl(GGUFTokenizerParameters, ...) currently ignores enable_save_ov_model (it only prints to stderr) because it has no directory to write to. Since enable_save_ov_model is an explicit user request, it would be better to fail fast with a clear exception instead of silently continuing without producing the requested artifacts.
// This constructor is given the metadata and no path, so there is nowhere to write the
// tokenizer/detokenizer IRs -- the empty save_dir below disables saving. LLMPipeline avoids
// that by routing to the from-file constructor whenever saving was requested, but a caller
// using the Tokenizer API directly would otherwise get no IRs and no indication why.
if (gguf_props.enable_save_ov_model) {
std::cerr << "[GGUF] enable_save_ov_model was requested but this Tokenizer was constructed from GGUF "
"tokenizer parameters, which carry no source directory to write to, so "
"openvino_tokenizer.xml / openvino_detokenizer.xml are NOT written. Construct Tokenizer "
"from the .gguf path instead."
<< std::endl;
| #include <algorithm> | ||
| #include <limits> | ||
| #include <cstdint> | ||
| #include <set> | ||
|
|
…ata not a model
Two API problems, both about asking for the wrong thing.
The reader choice was an environment variable, OPENVINO_GENAI_GGUF_LEGACY_READER.
It is a per-pipeline decision that changes what graph gets built and how the model
behaves, so it belongs with the other construction properties, not in the process
environment where it silently applies to every pipeline and cannot be set by a
caller that does not own the process. Replace it with GGUF_READER
("FRONTEND" default / "LEGACY"), modelled on ATTENTION_BACKEND ("PA"/"SDPA"):
a declared property, value constants next to PA_BACKEND/SDPA_BACKEND, and
validation that rejects anything else instead of silently falling back.
extract_gguf_properties() now returns a struct rather than a pair, so the reader
travels with enable_save_ov_model and both are stripped in one place. That is
load-bearing: these configure conversion, not inference, and the plugin rejects
unknown properties. Doing so surfaced a pre-existing leak on the
speculative-decoding path, where StatefulPipeline::create() copies the caller's map
into ModelDesc and the wrapper pipelines hand it to compile_model verbatim --
enable_save_ov_model already escaped there. Strip in create(), which fixes both.
The Tokenizer took a std::shared_ptr<ov::Model>. It never wanted a model: it dug
the GGUF tokenizer metadata out of the model's rt_info and used only that. Taking a
model implies any model works, when a plain IR, a model whose rt_info was stripped,
or one built by the legacy reader cannot produce a tokenizer at all -- and the
failure surfaced from inside a Tokenizer constructor, far from the mistake. Take
GGUFTokenizerParameters instead, so the constructor asks for exactly what it
consumes, and add GGUFTokenizerParameters::from_model() so the lookup -- and its
error, which now names the likely causes and the fix -- happens where the caller
reaches for the model.
Verified end to end against a real .gguf: FRONTEND, LEGACY and the default each
build a pipeline and generate, LEGACY visibly diverging (it ignores
rope_freqs.weight, so llama-3 RoPE scaling is not applied); an invalid GGUF_READER
is rejected; neither GGUF key reaches the plugin; from_model() + Tokenizer(params)
round-trips encode/decode; and a model without the metadata is refused with an
actionable message.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Adopt the native
.ggufbuilder from the OpenVINO GGUF frontend, so a.ggufis converted by the frontend instead of GenAI's own reader, and build the tokenizer from the converted model.rt_infoinstead of re-parsing the file: SPM and BPE flavours, special-token ids,add_space_prefix, plusgemma4andplamo2.CombineSegmentson every path. Two defects madeencode()disagree with llama.cpp: the BPE paths never readtokenizer.ggml.add_bos_token(BOS-sensitive models such as muse-glimmer and llama3 were silently fed a BOS-less prompt and degenerated), andadd_special_tokens=falsewas a no-op on both paths, becauseMakeAddSpecialTokensSatatefulneeds aCombineSegmentsnode that the GGUF tokenizer never built. Defaults followllama-vocab.cppwhen the key is absent.MakeStatefulas aDecoderTransformationExtension, scoped to this one conversion rather than globally viaov::Core::add_extension.gguf_arch_checkand a chat sample; measure prefill (not cache hits) in the GGUF perf sample.test_cli_text_gguf.pyplusmodel_loaderssupport for.ggufinputs.Warning
Draft — two unmerged dependencies:
thirdparty/openvino_tokenizersbump, [Tokenizers] Register RegexNormalization and Sentencepiece ops in the op factory openvino_tokenizers#724 (registersRegexNormalizationand the Sentencepiece ops in the op factory, needed by the gemma4 tokenizer). The submodule is pinned to that PR's commit until it merges.CVS-188634
Checklist: