Enable model-selected Engine chat batching - #1071
Conversation
Use each model's genai_config batching contract so independent chat sessions can share an ORT GenAI Engine while Generator models retain their existing behavior. Files changed: - Add the owner-thread Engine dispatcher and per-session adapter. - Route chat creation, continuation, cancellation, usage, and option changes through the selected backend. - Parse and validate static and dynamic Engine configuration. - Add configuration, template, concurrency, and search-option coverage. - Pin ORT GenAI and packaging to 0.15.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b90e66e-af55-404d-b606-7899f6f73c54
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Remove compatibility branches for OGA releases without Engine support so missing APIs fail during compilation instead of at model load. Files changed: - sdk_v2/cpp/CMakeLists.txt - sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc - sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b90e66e-af55-404d-b606-7899f6f73c54
Preserve the OGA 0.15.3 release requirement while adopting main's nightly GenAI validation path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b90e66e-af55-404d-b606-7899f6f73c54
Retain OGA 0.15.2 until the next Engine-capable stable package is published, while non-release CI continues using the selected nightly. Remove optional OGA hooks from the shared test entry point because not every test target consumes OGA. Files changed: - .pipelines/foundry-local-packaging.yml - .pipelines/v2/sdk_v2-pipeline-plan.md - sdk_v2/deps_versions.json - sdk_v2/cpp/test/test_main.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b90e66e-af55-404d-b606-7899f6f73c54
The shared test entry point is compiled by targets with and without OGA include paths. Retain header detection so OGA-linked tests shut down cleanly without imposing that dependency on cache-only tests. Files changed: - sdk_v2/cpp/test/test_main.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b90e66e-af55-404d-b606-7899f6f73c54
There was a problem hiding this comment.
🟡 Changes recommended
Release dependencies cannot compile the new API, and Engine failure and finish-reason handling contain correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds model-configured static or dynamic ORT GenAI Engine batching while preserving the existing Generator backend.
Changes:
- Parses Engine batching configuration and selects the appropriate backend.
- Adds a model-owned threaded Engine dispatcher with retained dynamic conversations.
- Extends chat generation, cancellation, usage accounting, templates, and tests.
File summaries
| File | Description |
|---|---|
sdk_v2/cpp/CMakeLists.txt |
Registers Engine sources. |
sdk_v2/cpp/src/inferencing/generative/genai_config.h |
Defines Engine configuration. |
sdk_v2/cpp/src/inferencing/generative/genai_config.cc |
Parses and validates batching modes. |
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.h |
Stores the model-owned Engine. |
sdk_v2/cpp/src/inferencing/generative/genai_model_instance.cc |
Creates and destroys the Engine. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_generator.h |
Generalizes retained generator operations. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_generator.cc |
Adds optional turn usage. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_engine.h |
Declares the Engine dispatcher. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_engine.cc |
Implements scheduling and event routing. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_generator.h |
Declares the Engine adapter. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_generator.cc |
Implements Engine-backed generation. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_generator.h |
Adapts the Generator interface. |
sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_generator.cc |
Shares guidance handling. |
sdk_v2/cpp/src/inferencing/generative/chat/search_options.h |
Adds retained-setting and limit helpers. |
sdk_v2/cpp/src/inferencing/generative/chat/search_options.cc |
Resolves limits and guidance. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_template.h |
Declares continuation prompt support. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_template.cc |
Derives assistant-turn boundaries. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_session.h |
Stores backend-neutral retained state. |
sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc |
Selects and manages chat backends. |
sdk_v2/cpp/test/test_main.cc |
Shuts down ORT GenAI after tests. |
sdk_v2/cpp/test/internal_api/genai_config_test.cc |
Tests Engine configuration parsing. |
sdk_v2/cpp/test/internal_api/chat/search_options_test.cc |
Tests limits and retained settings. |
sdk_v2/cpp/test/internal_api/chat/chat_template_test.cc |
Tests continuation boundaries. |
sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc |
Adds concurrent-session coverage. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merge origin/main to resolve the PR conflict without rewriting history. Engine events: fail all conversations when an engine-level failure has no request handle; propagate native finish reasons so implicit output and session limits report LENGTH. C++ cleanup: wrap the long Engine validation message and remove duplicate gtest includes. Files changed: merged main updates; Engine chat dispatcher/generator/session metadata; GenAI model instance; test main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The pinned GenAI dependency cannot compile the new API, and request limiting, undo, and lifetime paths have correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 5
- Review effort level: Balanced
Destroy retained generators before releasing model ownership, preserve undo correctness after generator rebuilds, and keep the Generator path's implicit limit behavior unchanged. Use bounded Generator context for one-shot OpenAI JSON requests and map the CI nightly's native StopSequence finish reason. Stage the shared chat model with dynamic Engine batching so concurrency and cancellation coverage exercise the dispatcher. Files changed: chat_session.cc, chat_session.h, onnx_engine_chat_generator.cc, chat_session_test.cc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The current dependency pin and finish-reason symbol prevent a clean build, with additional retained-state correctness and cleanup issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc:515
- Retained requests are rebuilt only when the boolean guidance mode changes, but
ApplyGuidanceOptionsalso bakes the specificguidance_typeandguidance_datainto the request. Two tool-only turns with different response schemas therefore reuse the first turn's grammar and constrain the second response incorrectly. Include changes to those guidance values in the invalidation condition.
sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_generator.cc:172 - After
CreateConversationinserts the native request into the engine map, any exception fromBeginTurn, tokenizer-stream creation, or generator allocation exits withoutClose. The map retains the conversation and request indefinitely, potentially consuming batching/KV capacity after a failed request. Add failure cleanup until ownership has transferred to the returned generator.
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Balanced
Invalidate retained requests when explicit guidance changes, and close newly-created Engine conversations if turn setup or adapter construction fails. Accept both StopSequence and StopString finish-reason symbols while compiling against the exact non-release CI nightly. Files changed: chat_session.cc, onnx_engine_chat_generator.cc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bundle the exact native dependencies into non-release Python and JS artifacts so package tests do not preload the older stable OGA ABI. Treat terminated classic generators as non-rewindable so cancelled streams cannot poison the next turn. Files changed: packaging and v2 Python/JS pipeline templates; onnx_chat_generator.h; Python native loader and tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allow and require bundled ORT and GenAI binaries for non-release wheels while retaining the dependency-only invariant for stable release wheels. Files changed: .pipelines/v2/templates/steps-build-python.yml. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Thanks for the work here. The model-owned Engine and owner-thread design look like the right foundation for shared paged-KV coding workloads. I found a few items worth addressing before approval.
The main blocker is release packaging. The release path still pins ORT GenAI 0.15.2, while these Engine files use APIs that are only in the nightly today. NuGet does not publish 0.15.3 yet. I recommend either holding the merge until the stable package is available, or adding a compile-time Engine capability gate so 0.15.2 release builds remain Generator-only and Engine-configured models fail with a clear unsupported-runtime error. Please keep stable-0.15.2 and nightly-Engine CI lanes until activation.
For the coding scenario, the dynamic path also needs a real CI gate. The checked-in real-model tests use static Engine at two concurrent sessions. The manual A100 B4 result is useful, but I suggest adding a paged dynamic model test covering B1/B4/B8, retained continuation, capacity pressure, cancellation/recovery, output limits, and unload.
The packaging matrix is still running, so I would wait for it before approval.
Keep release artifacts on the declared 0.15.2 dependencies and let non-release CI select the Engine-capable nightly without bundling mismatched native runtimes into Python or JS packages. Files changed: - .pipelines/foundry-local-packaging.yml - .pipelines/v2/templates/stages-js.yml - .pipelines/v2/templates/stages-python.yml - .pipelines/v2/templates/stages-sdk-v2.yml - .pipelines/v2/templates/steps-build-js.yml - .pipelines/v2/templates/steps-build-python.yml - sdk_v2/python/src/foundry_local_sdk/_native/lib_loader.py - sdk_v2/python/test/unit/test_lib_loader.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep 0.15.2 builds on the Generator backend until the complete dynamic Engine API ships, while enabling Engine sources automatically for compatible nightlies. Address valid review findings by removing unsupported static batching, preserving Generator sampling defaults, making late cancellation harmless, retaining capacity-blocked requests, selecting idle eviction victims by least-recent activity, and documenting continuation and event-buffer behavior.
Files changed:
- sdk_v2/cpp/CMakeLists.txt
- sdk_v2/cpp/include/foundry_local/foundry_local_c.h
- sdk_v2/cpp/src/inferencing/generative/chat/*
- sdk_v2/cpp/src/inferencing/generative/genai_config.{cc,h}
- sdk_v2/cpp/src/inferencing/generative/genai_model_instance.{cc,h}
- sdk_v2/cpp/test/CMakeLists.txt
- sdk_v2/cpp/test/internal_api/chat/*
- sdk_v2/cpp/test/internal_api/genai_config_test.cc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Baiju Meswani (baijumeswani)
left a comment
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier runtime concerns. Leaving the packaging failures aside, I found a few remaining items that would make this safer for long-running coding sessions.
Use the 0.15.2 pin for every packaging build so JS and Python do not consume native binaries linked to unsupported Engine symbols. Reject malformed engine configuration instead of silently falling back to the Generator path. Files changed: - .pipelines/foundry-local-packaging.yml - .pipelines/v2/templates/steps-prefetch-nuget.yml - sdk_v2/cpp/src/inferencing/generative/genai_config.cc - sdk_v2/cpp/test/internal_api/genai_config_test.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use max_batch_size instead of undocumented two-times headroom because OGA retains speculative and fatal overflow for later Run calls. Files changed: - sdk_v2/cpp/src/inferencing/generative/chat/onnx_chat_engine.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid describing the model-owned Engine as a generator while retaining the shared ChatGenerator interface expected by ChatSession. Files changed: - sdk_v2/cpp/CMakeLists.txt - sdk_v2/cpp/src/inferencing/generative/chat/chat_session.cc - sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.cc - sdk_v2/cpp/src/inferencing/generative/chat/onnx_engine_chat_stream.h Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implement bounded capacity handling and cancellation recovery so retained sessions cannot hang or become unusable. Forward max output tokens only when callers supply them, preserving model/session context defaults. Document retained-state continuation behavior and add capability-gated dynamic Engine coverage for concurrency, capacity, cancellation, and unload. Files changed: Engine dispatcher and options, generator/session recovery, public/internal docs, CMake tests, and chat test suites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The implementation currently rejects engine.static_batching (despite the PR’s stated contract) and the packaging pipeline now hard-pins GenAI to 0.15.2 for all builds, undermining the described Engine validation path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 44/44 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new Engine continuation prompt test contains an ineffective marker-leak assertion (checks the wrong marker string), so test coverage for the intended behavior is currently incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 44/44 changed files
- Comments generated: 1
- Review effort level: Lite
Check the generated marker prefix so numeric sentinel suffixes cannot evade the continuation-prompt assertion. Files changed: sdk_v2/cpp/test/internal_api/chat/chat_template_test.cc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core chat inference execution (including new threaded Engine dispatch, retained-state semantics, and stop-string handling) across multiple languages and build gates, warranting final human review despite added tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdk_v2/cpp/CMakeLists.txt:137
- The dynamic-Engine capability probe compiles only if
OgaTurnOptions::SetRepetitionPenaltyexists, but the Foundry code path explicitly rejects nonzero frequency/presence penalties and never calls this API. Keeping it in the probe can create false negatives (Engine disabled) on runtimes that otherwise support the required Engine surface but omit/rename this unrelated setter. Remove this line so the probe reflects the APIs actually required by the implementation.
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
- Review effort level: Lite
Remove the unused repetition-penalty setter from the capability check so compatible runtimes are not disabled by an unrelated API omission or rename. Files changed: sdk_v2/cpp/CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the latest suppressed capability-probe finding in 4c53645: |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a substantial new backend execution path (Engine dispatcher + retention/cancellation/capacity semantics) with broad cross-cutting changes, warranting final human review despite targeted tests.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
sdk_v2/cpp/include/foundry_local/foundry_local_c.h:302
- This macro comment states early_stopping only supports false, but Generator-backed sessions still accept and forward early_stopping=true. Update the comment to clarify it’s supported on Generator backends and rejected by Engine backends when true.
sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h:949 - The public C++ header comment says early_stopping only supports false, but Generator-backed sessions still accept and forward early_stopping=true (see ApplySearchOptions / tests). This doc comment should reflect that it’s Generator-only and Engine backends reject true, rather than implying the option is unusable.
- Files reviewed: 44/44 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Final Copilot pass generated 0 new inline comments. Regarding its two suppressed |
Avoid synthesizing an output limit for Engine text turns while retaining classic Generator and media defaults. Restore unrelated pipeline and C ABI edits to the PR base and clarify the C++ early-stopping contract. Files changed: pipeline dependency configuration, C/C++ public headers, chat session limit handling, and search option documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve correct prompt, tool, and per-turn state when reusing Generator and Engine chat backends. Add cancellation recovery and dynamic Engine regression coverage. Files changed: C ABI header; chat generator/session/template and Engine implementations; chat unit and integration tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Baiju Meswani (baijumeswani)
left a comment
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
Correctness review of the current Engine integration. I found two usage-accounting issues that should be addressed before the Engine path is enabled for release; omitting the separate CI-lane concern as requested.
Summary
genai_config.jsonChatSessionsEngine integration details
Missing assistant-turn boundary
ORT GenAI Engine does not append or retain a generated EOS token in the request's logical sequence. A later
BeginTurnmust therefore include the model-specific tokens that close the prior assistant turn before introducing the next user turn. The Generator path already retains EOS, so reusing its append fragment caused dynamic Engine sessions to lose conversational context even though the request and KV cache remained resident.This change derives the continuation boundary from the model's own chat template using a collision-free synthetic assistant marker, removes everything through that marker before tokenization, and submits only the model-specific assistant boundary plus the new-turn fragment. This avoids hard-coding token IDs or template syntax and avoids injecting fresh-conversation BOS/default-system framing into retained state.
Per-turn output limits
When callers explicitly provide
max_output_tokens, every Engine turn forwards that value throughOgaTurnOptions, validates it against the retained-plus-new input context, and waits for the corresponding terminal event. When callers omitmax_output_tokens, Foundry does not invent or forward a default Engine turn limit; OGA enforces the Request/session context ceiling.Engine usage and finish reasons come from terminal events. Generator-backed requests retain their established host-side default and sampling behavior.
Capacity and cancellation
Completed dynamic requests remain resident for KV-cache reuse and are evicted least-recently-used when capacity is needed. If every resident request is active, the dispatcher continues servicing cancellation/close commands and waits up to a bounded deadline. Expiration cancels and closes only the newest waiting no-progress conversation rather than poisoning the model-owned Engine or active peers.
A late cancellation after completion is a no-op. Canceled turns are not committed to history: retained state is rewound when the selected inference path can restore its pre-turn position, otherwise it is discarded and rebuilt from committed history on the next request.
Backend contract
engine.dynamic_batchingengine.static_batchingengineobjectengineobjectEngine selection is limited to text chat models. Multimodal Engine declarations fail at model load rather than failing later during a request.
Runtime compatibility
The repository and release pipeline remain pinned to
Microsoft.ML.OnnxRuntimeGenAI.Foundry0.15.2. CMake probes for the dynamic Engine API and excludes Engine-only sources/tests when it is unavailable, so current release builds remain Generator-only. The future implementation was also compiled against cached0.16.0-dev1001407373, where the capability probe succeeds.An Engine-enabled CI lane must stage a real paged-KV model and set
FOUNDRY_LOCAL_DYNAMIC_ENGINE_TEST_MODEL_PATH; the test policy documents this requirement so an enabled lane cannot silently rely on the conventional shared-buffer fixture.Validation
0.16.0-dev1001407373, compiling the Engine implementation and dynamic test suiteFOUNDRY_LOCAL_DYNAMIC_ENGINE_TEST_MODEL_PATHwas not set