Custom KV input names & types - #3
Merged
Merged
Conversation
Make logits & KV cache type be settable in config
Ryan Hill (RyanUnderhill)
requested review from
aciddelgado and
kunal-vaishnavi
January 18, 2024 22:00
aciddelgado
approved these changes
Jan 18, 2024
Nat Kershaw (MSFT) (natke)
added a commit
that referenced
this pull request
Apr 23, 2024
Co-authored-by: Ye Wang <52801275+wangyems@users.noreply.github.com> Co-authored-by: Patrice Vignola <vignola.patrice@gmail.com> Co-authored-by: Ryan Hill <38674843+RyanUnderhill@users.noreply.github.com>
Justin Chu (justinchuby)
added a commit
to justinchuby/onnxruntime-genai
that referenced
this pull request
May 2, 2026
…ntermediates Fix 2 remaining review findings: 1. FlowInterpreter is now fully stateless — no mutable intermediates map. GetWiredInputs takes an externally-owned intermediates map as a parameter. This prevents clobbering when multiple States share a Model (finding microsoft#3). 2. NonDecoderSessionIO stores owned std::string copies instead of raw c_str() pointers from ExtraInput, preventing dangling pointer bugs if the ExtraInput vector is relocated (finding microsoft#4). Intermediate tensor ownership is now fully in PipelineConfigState: - intermediate_owned_: unique_ptr<OrtValue> map (memory ownership) - intermediates_: raw OrtValue* map (fast lookup for wiring) Both are per-State, not per-Model. Tests updated to match stateless FlowInterpreter API. All 78 tests pass (9 skipped, GPU-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Justin Chu <justinchu@microsoft.com>
Justin Chu (justinchuby)
added a commit
to justinchuby/onnxruntime-genai
that referenced
this pull request
May 2, 2026
…ntermediates Fix 2 remaining review findings: 1. FlowInterpreter is now fully stateless — no mutable intermediates map. GetWiredInputs takes an externally-owned intermediates map as a parameter. This prevents clobbering when multiple States share a Model (finding microsoft#3). 2. NonDecoderSessionIO stores owned std::string copies instead of raw c_str() pointers from ExtraInput, preventing dangling pointer bugs if the ExtraInput vector is relocated (finding microsoft#4). Intermediate tensor ownership is now fully in PipelineConfigState: - intermediate_owned_: unique_ptr<OrtValue> map (memory ownership) - intermediates_: raw OrtValue* map (fast lookup for wiring) Both are per-State, not per-Model. Tests updated to match stateless FlowInterpreter API. All 78 tests pass (9 skipped, GPU-only). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Justin Chu <justinchu@microsoft.com>
anilmartha
referenced
this pull request
in anilmartha/onnxruntime-genai
May 18, 2026
- builder.py: introduce _has_vcf_architecture() helper that handles architectures as a list, a bare string, or absent/unknown type, then use it in both the local-dir and HF-hub code paths (Copilot comment #2) - config.h: correct misleading comment on num_visual_tokens — remove the false "0 = compute from image_grid_thw" claim; the field must be > 0 for videochat_flash_qwen (Copilot comment #3) - videochat_flash_processor.cpp: add clarifying comment to empty catch block — exception is expected when only the language decoder session is loaded (Copilot comment microsoft#4) - builders/__init__.py: move VideoChatFlashQwenModel to its correct alphabetical position in __all__ (after SmolLM3Model, before WhisperModel) per kunal-vaishnavi comment microsoft#6 Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Baiju Meswani (baijumeswani)
pushed a commit
that referenced
this pull request
Jul 16, 2026
## Fix heap-buffer-overflow in `GenerateNextToken` when sequence is at `max_length` ### Summary Adds a fast-fail guard at the entry of `Generator::GenerateNextToken()` that throws a clear `std::runtime_error` when the sequence has already reached `max_length`. This prevents a heap-buffer-overflow write into the preallocated sequences buffer that is otherwise reachable via the C API. ### Reported bug (AddressSanitizer) A libFuzzer harness driving the `OgaGenerator_*` C API triggered the following crash: ``` ==ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 4 #0 Generators::GreedySearch_Cpu::AppendNextTokensToSequences() src/search.cpp:323 #1 Generators::GreedySearch_Cpu::SampleTopK(int, float) src/search.cpp:192 #2 Generators::Generator::GenerateNextToken() src/generators.cpp:602 #3 OgaGenerator_GenerateNextToken src/ort_genai_c.cpp:473 #4 <fuzzer harness> 0 bytes after 64-byte region ← writes at index max_length allocated by Generators::Sequences::Sequences(...) at construction ``` ### Root cause Inside `GreedySearch_Cpu::AppendTokens`, `ResetDone()` is called unconditionally at the end. If the previous call filled the sequences buffer up to exactly `max_length`, `done_` is cleared. A subsequent `GenerateNextToken()` then: 1. Enters `SampleTopK()`. 2. The `!done_` check falsely passes (buffer is actually full). 3. `AppendNextTokensToSequences()` writes at index `max_length` → OOB write into the redzone immediately following the 64-byte sequences buffer. ### Fix Add an early runtime error at the top of `Generator::GenerateNextToken()`: ```cpp if (search_->GetSequenceLength() >= state_->params_->search.max_length) throw std::runtime_error( "GenerateNextToken called with sequence length already at max_length (" + std::to_string(state_->params_->search.max_length) + ")"); ``` This sits directly above frame #2 of the crash stack, so the OOB path in frames #0 and #1 is unreachable. The exception propagates through `OgaGenerator_GenerateNextToken` and is surfaced to the caller as a normal API error instead of memory corruption.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moved model specific config stuff to a sub-object "model".
Made all KV input/output names settable through the config, no longer hardcoded by model type
Made the KV cache tensor type settable in config (float16/float32)
Also made logits tensor type settable in config (float16/float32)
Mistral handler is now identical to Llama, but might diverge with other Mistral specific features.