Samuel100/sdkv2 rust - #836
Open
samuel100 wants to merge 13 commits into
Open
Conversation
Replace the owned-Self FoundryLocalManager::create with an Arc<Self> that shares one process-wide instance via a static OnceLock<Mutex<Weak<...>>>. While any handle is alive, callers share the same instance; when the last Arc drops, NativeManager::Drop runs teardown (Manager_Shutdown + Manager_Release) while the ORT runtime is still alive and before the library's C++ static destructors -- restoring singleton semantics without the atexit hook that caused the WebGPU ReleaseEpFactory double-unregister (ORT #29206). Use OnceLock to wrap the Mutex (const Weak::new() needs Rust 1.73, above the crate's 1.70 MSRV). Update stale atexit/singleton doc comments in manager.rs, foundry_local_manager.rs, and docs/api.md. Keep the test helper's &'static signature by holding a process-lifetime OnceLock<Arc<...>>, so all existing call sites compile unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…elease Fix a use-after-free reachable from safe code: Model, the OpenAI clients, and sessions held only a raw flModel*/flSession* plus Arc<Api> (which keeps just the shared library loaded), so dropping the last Arc<FoundryLocalManager> released the native catalog and model handles while those derived objects were still alive. A natural factory pattern (create manager, get a client, return it) would dangle. This was latent under the old leaked &'static singleton and became reachable once teardown moved onto last-Arc Drop. Thread a strong Arc<NativeManager> keep-alive through NativeModel, NativeCatalog, and NativeSession so every derived handle keeps the native manager (and thus the catalog/model handles it owns) alive until the handle itself is dropped. The keep-alive targets NativeManager (a leaf that owns no Rust wrappers), so there is no reference cycle. Also add NATIVE_LIFECYCLE, a Mutex<()> held across both Manager_Create and Manager_Release, to close the create/teardown race: an Arc's strong count reaches zero slightly before Drop finishes Manager_Release, so a concurrent create() could otherwise observe no instance yet be rejected by the native single-instance guard. Validated: cargo fmt/check/clippy/doc clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
origin/main #746 (Add speech result types) inserted GetSpeechSegment and GetSpeechResult into flItemApi between GetToolResult and GetMetadata. Because ffi.rs mirrors the vtable positionally via #[repr(C)], the old layout left GetMetadata/GetMutableMetadata/GetQueue and all ItemQueue_* slots shifted by two pointers — so calls like ItemQueue_Push/TryPop (used by streaming) would misdispatch to the wrong native function against a core built from the new header. Add the two function-pointer slots in the matching position, plus the supporting flSpeechWord/flSpeechSegmentData/flSpeechResultData structs, flSpeechSegmentKind, the FOUNDRY_LOCAL_ITEM_SPEECH_SEGMENT/RESULT item-type constants, and the DURATION/CONFIDENCE_UNSET sentinels. API version is unchanged (still 1). Validated: cargo fmt/check/clippy clean (lib, tests, examples). Vtable order now matches foundry_local_c.h flItemApi field-for-field (31 entries). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
origin/main #746 changed the native audio path to emit SPEECH_SEGMENT items (streaming) and a SPEECH_RESULT item (final), replacing the TextItem outputs. LiveAudioTranscriptionSession uses that native path but read results only via read_text_item / item_text (ITEM_TEXT only), so against the new core it silently produced empty streaming results and an empty final transcript. Add read_speech_segment / read_speech_result_text helpers (via the new GetSpeechSegment / GetSpeechResult accessors) and wire them into the streaming trampoline and final-transcript aggregation, with a TEXT fallback so the OpenAI-JSON path and older cores keep working. Segment timing (ms→s) and PARTIAL/FINAL state are mapped onto the existing response envelope. AudioClient::transcribe / transcribe_streaming are unaffected — they use the OpenAI-JSON path, which still returns OPENAI_JSON-tagged TEXT items. Validated: cargo fmt/check/clippy clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ports the Foundry Local Rust SDK (sdk_v2/rust/) onto the new C++ Core ABI exposed through foundry_local_c.h. It rebuilds the public surface (FoundryLocalManager, Catalog, Model, OpenAI-style chat/embedding/audio clients, and a live-audio streaming session) on top of thin extern "system" FFI wrappers, with native-handle lifetimes anchored to a shared Arc<NativeManager>, and adds a single-binary integration test suite plus generated API docs.
Changes:
- New FFI-backed core wrappers (
detail/{api,native,manager,session,items,model,info,...}) that mirror the C ABI's ownership model (Manager owns Catalog/Models; handles keep the Manager alive; item-queue/request ownership transfer rules). - New OpenAI-compatible clients (
openai/{chat_client,embedding_client,audio_client,live_audio_session}) and public types, re-exported fromlib.rs. - A consolidated integration test binary (
tests/integration/*), examples,Cargo.toml/build.rsupdates, and API reference docs.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
sdk_v2/rust/src/openai/live_audio_session.rs |
Live audio streaming session; exposes push_queue_capacity/bits_per_sample options that aren't wired to the native layer (commented). |
sdk_v2/rust/src/detail/{api,native,manager,session,items,model,info}.rs |
Core FFI wrappers and native lifetime/ownership handling; verified against the C ABI and stored conventions. |
sdk_v2/rust/src/openai/{chat_client,embedding_client,audio_client,mod}.rs |
OpenAI-compatible clients and response/streaming handling. |
sdk_v2/rust/src/{lib,catalog,configuration,foundry_local_manager,types,error}.rs |
Public API surface and re-exports (Model, DownloadBuilder, etc.). |
sdk_v2/rust/tests/integration/common/mod.rs |
Shared test config; logsDir still points at the legacy sdk/rust tree (commented). |
sdk_v2/rust/tests/integration/{main,manager,model,web_service,live_audio,embedding_client,chat_client}_test.rs |
New consolidated integration tests; minor duplicate/mislabeled log line in chat test (commented). |
sdk_v2/rust/GENERATE-DOCS.md, sdk_v2/rust/docs/api.md |
Doc generation guide and API reference; reference the legacy sdk/rust path and a nonexistent ModelVariant type (commented). |
sdk_v2/rust/Cargo.toml, sdk_v2/rust/build.rs |
Package metadata and native-artifact build script (verified). |
nenad1002
reviewed
Jun 24, 2026
nenad1002
reviewed
Jun 24, 2026
Contributor
|
All Copilot comments look actionable as well |
Docs/paths (Copilot): - test logsDir, GENERATE-DOCS.md, docs/api.md: legacy sdk/rust -> sdk_v2/rust - GENERATE-DOCS.md: replace nonexistent ModelVariant row with DownloadBuilder Tests (Copilot): - chat_client_test: drop the redundant, mislabeled "REST response" print Model API (nenad1002): - unload/remove_from_cache now return Result<()> (no empty-String sentinel) - ModelKind stores Arc<VariantData> to avoid deep VariantData/NativeModel clones - create_chat/audio/embedding_client read selected_variant() once so a concurrent select_variant_by_id can't mismatch id vs native - fix the single-variant select error text; method name no longer misstated Catalog (nenad1002): - update_models doc states plainly it is a no-op (native self-refreshes) Live audio (Copilot + nenad1002): - remove unwired bits_per_sample and push_queue_capacity options and the false backpressure claim (append() is unbounded); update the live-audio test - add Drop to LiveAudioTranscriptionSession: best-effort mark the input queue finished via get_mut so the blocking worker drains and the native session is released if the session is dropped without stop() Configuration (nenad1002): - logger() doc states plainly it is not wired to native (no logger-callback ABI; use log_level/logs_dir), kept for forward compatibility build.rs (nenad1002, nit): - integrity guard: reject empty payloads and require the expected native file to be present after extraction. Cryptographic SHA-512-vs-registration is feed- specific (nuget.org vs Azure DevOps) and left as future hardening. Validated: cargo fmt/check/clippy/doc clean (lib, tests, examples). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
skottmckay
reviewed
Jul 9, 2026
Comment on lines
+15
to
+25
| - [OpenAI Clients](#openai-clients) | ||
| - [ChatClient](#chatclient) | ||
| - [ChatCompletionStream](#chatcompletionstream) | ||
| - [EmbeddingClient](#embeddingclient) | ||
| - [EmbeddingResponse](#embeddingresponse) | ||
| - [AudioClient](#audioclient) | ||
| - [AudioTranscriptionStream](#audiotranscriptionstream) | ||
| - [AudioTranscriptionResponse](#audiotranscriptionresponse) | ||
| - [TranscriptionSegment](#transcriptionsegment) | ||
| - [TranscriptionWord](#transcriptionword) | ||
| - [JsonStream\<T\>](#jsonstreamt) |
Collaborator
There was a problem hiding this comment.
Is there a plan to add the new Session/Item/Request/Response setup? We are making all these direct OAI APIs as deprecated in the other language bindings.
Ports the modality-agnostic inference surface from the C++/C#/Python/JS bindings to the Rust SDK, following idiomatic Rust: - Pure-data owned values: `Item` (enum + Text/Message/Tensor/Image/Audio/ ToolCall/ToolResult/Speech* variants and supporting types), `Request`, `Response` — all Send+Sync+Clone with no native handle. - Handle-backed types shared via `Arc`: `Session` (+ typed `ChatSession`, `EmbeddingsSession`, `AudioSession` which Deref to `Session`) and `ItemQueue`. - Streaming via `futures_core::Stream` (`ItemStream`) instead of a callback setter; dropping the stream cancels generation. - `process_request` is cancel-on-drop: a `CancelGuard` fires the native `Request_Cancel` if the future is dropped before completion (e.g. via `tokio::time::timeout` / `select!`), matching every other binding's `Request::Cancel` while staying idiomatic. - Convenience helpers: `ChatSession` tool + turn management, `EmbeddingsSession::embed`/`embed_batch`, `AudioSession::transcribe`. Adds native bridge (`detail/items.rs`, `detail/session.rs`), FFI param/dtype constants, a low-level example, integration tests, and API docs. Renames the OpenAI facade's `FinishReason` re-export to `ChatFinishReason` to avoid colliding with the core inference `FinishReason`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
…ts (Rust)
Correct the Rust OpenAI direct chat client so its request JSON matches the
authoritative native `*-json` contract (chat_completions_converter.cc):
- response_format keys use snake_case (`json_schema`, `lark_grammar`) — the
previous camelCase keys were silently dropped by the native converter.
- tool_choice emits plain strings ("none"/"auto"/"required") and a nested
`{"type":"function","function":{"name":…}}` for a named function, matching
what the converter parses. Add regression unit tests locking in the shapes.
Deprecate the OpenAI direct clients (ChatClient, AudioClient, EmbeddingClient,
LiveAudioTranscriptionSession) and their factories with
`#[deprecated(since = "2.0.0")]`, pointing users to the Session API
(ChatSession/AudioSession/EmbeddingsSession), matching the C#/JS/Python SDKs.
Facade modules, re-exports, examples and integration tests opt out via
`#[allow(deprecated)]` to keep the `-D warnings` build green. Update docs/api.md.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
Migrate the bundled Rust SDK examples off the deprecated OpenAI-facade chat client onto the idiomatic Session/Item/Request/Response API: - chat_completion.rs: ChatSession::new + set_options; sync process_request -> response.text(); streaming via process_streaming_request + as_text(). - interactive_chat.rs: stateful loop; system prompt seeded only when turn_count() == 0; no manual message history. - tool_calling.rs: register the tool once via add_tool_definition; first turn tool_choice=Required streams complete Item::ToolCall values; feed back only Item::tool_result; second turn tool_choice=None for the final answer. Removes the manual streamed-tool-call delta reassembly. Drops #![allow(deprecated)] from all three; they no longer touch the facade. Verified: cargo clippy --all-targets -- -D warnings, cargo fmt --check, cargo build --examples, cargo test --lib (14/14) all clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d411120b-849b-4025-823b-59a69a3a563f
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (4)
sdk_v2/rust/tests/integration/session_test.rs:110
- The native core refuses to unload a model while any session still references it.
sessionremains alive until the end of this test, so thisunload()is guaranteed to return “session(s) still using it” and fail the test. Drop the session first.
sdk_v2/rust/tests/integration/session_test.rs:131 - The native core refuses to unload a model while any session still references it.
sessionremains alive until the end of this test, so thisunload()is guaranteed to return “session(s) still using it” and fail the test. Drop the session first.
sdk_v2/rust/tests/integration/session_test.rs:169 - The embeddings session still owns a live native session here, and the core rejects unloading models with live sessions. Drop
sessionbefore unloading so this test can complete successfully.
sdk_v2/rust/examples/tool_calling.rs:144 sessionstill owns the native chat session here, so the core rejects this unload with “session(s) still using it.” Drop the session before model cleanup.
model.unload().await?;
Comment on lines
+272
to
+276
| inner.add_tool_definition( | ||
| &definition.name, | ||
| definition.description.as_deref(), | ||
| &definition.json_schema, | ||
| ) |
| homepage = "https://www.foundrylocal.ai/" | ||
| repository = "https://github.com/microsoft/Foundry-Local" | ||
| documentation = "https://github.com/microsoft/Foundry-Local/blob/main/sdk_v2/rust/docs/api.md" | ||
| include = ["src/**", "build.rs", "Cargo.toml", "README.md", "LICENSE.txt", "deps_versions.json"] |
Comment on lines
+142
to
+145
| let name_cstrings: Option<Vec<std::ffi::CString>> = names.map(|ns| { | ||
| ns.iter() | ||
| .filter_map(|n| std::ffi::CString::new(*n).ok()) | ||
| .collect() |
| let message = | ||
| native.download_and_register_eps(name_refs.as_deref(), progress, cancel_flag); | ||
| // Re-query registration state to synthesise the per-EP result. | ||
| let after = native.discover_eps().unwrap_or_default(); |
Comment on lines
+3
to
+7
| //! All test modules are compiled into one binary so the native core is only | ||
| //! initialised once (via the `OnceLock` singleton in `FoundryLocalManager`). | ||
| //! Running them as separate binaries causes "already initialized" errors | ||
| //! because the .NET native runtime retains state across process-level | ||
| //! library loads. |
| println!("\nCompleted {} turn(s).", session.turn_count()); | ||
|
|
||
| // ── 6. Unload the model ────────────────────────────────────────────── | ||
| model.unload().await?; |
|
|
||
| // ── Cleanup ────────────────────────────────────────────────────────── | ||
| println!("\nUnloading model…"); | ||
| model.unload().await?; |
|
|
||
| if tool_results.is_empty() { | ||
| println!("(model did not request any tool calls)"); | ||
| model.unload().await?; |
Comment on lines
+270
to
+275
| _ => native | ||
| .discover_eps() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|e| e.name) | ||
| .collect(), |
Comment on lines
+188
to
+192
| pub async fn start_web_service(&self) -> Result<()> { | ||
| let native = Arc::clone(&self.native); | ||
| let urls = spawn_blocking(move || { | ||
| native.web_service_start()?; | ||
| native.web_service_urls() |
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
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.
Rust SDK updated to new C++ Foundry Local Core ABI.