[Draft] Propagate cancellation to model and EP downloads#674
[Draft] Propagate cancellation to model and EP downloads#674
Conversation
Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/21906197-0d60-44b4-b5e6-4698c0fecfe3 Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
…ovided Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/21906197-0d60-44b4-b5e6-4698c0fecfe3 Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds download cancellation support across SDKs by routing model and EP downloads through streaming/callback interop when a cancellation signal/token/flag is supplied, and documenting the new behavior.
Changes:
- Rust: add cancellable variants of model/EP download APIs and cancellable streaming interop in
CoreInterop. - Python/JS/C#: add cancellation parameters (Event/AbortSignal/CancellationToken) and ensure downloads use callback/streaming paths to observe cancellation.
- Docs/tests: document cancellation usage and add unit tests verifying the callback/streaming path is used when cancellation is provided.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/rust/src/foundry_local_manager.rs | Adds cancellable EP download APIs and routes to cancellable streaming interop. |
| sdk/rust/src/detail/model_variant.rs | Adds cancellable model variant download path via cancellable streaming interop. |
| sdk/rust/src/detail/model.rs | Exposes cancellable model download API. |
| sdk/rust/src/detail/core_interop.rs | Adds cancellable streaming interop and cancellation checks in the FFI callback trampoline. |
| sdk/rust/README.md | Documents Rust cancellation usage for downloads. |
| sdk/python/test/test_model.py | Adds a test ensuring model download uses callback path when cancel_event is provided. |
| sdk/python/test/test_foundry_local_manager.py | Adds a test ensuring EP download uses callback path when cancel_event is provided. |
| sdk/python/src/imodel.py | Extends download() interface with optional cancel_event. |
| sdk/python/src/foundry_local_manager.py | Adds cancel_event support and routes through callback interop when provided. |
| sdk/python/src/detail/model_variant.py | Adds cancel_event support and routes through callback interop when provided. |
| sdk/python/src/detail/model.py | Plumbs cancel_event through Model.download() to the selected variant. |
| sdk/python/src/detail/core_interop.py | Adds cancellation support to callback helper and raises on cancellation. |
| sdk/python/README.md | Documents Python cancellation usage for downloads. |
| sdk/js/test/model.test.ts | Adds a test ensuring model download uses streaming interop when only AbortSignal is provided. |
| sdk/js/test/foundryLocalManager.test.ts | Adds a test ensuring EP download passes AbortSignal through to streaming interop. |
| sdk/js/src/imodel.ts | Extends download() interface with optional AbortSignal. |
| sdk/js/src/foundryLocalManager.ts | Adds AbortSignal overloads and passes signal to streaming interop. |
| sdk/js/src/detail/modelVariant.ts | Routes model download to streaming when progress or cancellation is needed. |
| sdk/js/src/detail/model.ts | Plumbs AbortSignal through Model.download() to the selected variant. |
| sdk/js/src/detail/coreInterop.ts | Adds AbortSignal-aware streaming wrapper that rejects with AbortError on cancellation. |
| sdk/js/README.md | Documents JS cancellation usage for downloads. |
| sdk/cs/test/FoundryLocal.Tests/Utils.cs | Improves repo-root discovery to handle .git files (worktrees/submodules). |
| sdk/cs/test/FoundryLocal.Tests/DownloadCancellationTests.cs | Adds a test covering cancellation propagation via callback path in model download. |
| sdk/cs/src/FoundryLocalManager.cs | Routes EP download through callback path when cancellation is possible; checks token in callback. |
| sdk/cs/src/Detail/ModelVariant.cs | Routes model download through callback path when cancellation is possible; checks token in callback. |
| sdk/cs/src/Detail/CoreInterop.cs | Preserves OperationCanceledException propagation from callback execution. |
| sdk/cs/README.md | Documents C# cancellation usage for downloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| self = ctypes.cast(self_ptr, ctypes.POINTER(ctypes.py_object)).contents.value | ||
|
|
||
| # Check for cancellation before processing the callback data. | ||
| if self._cancel_event is not None and self._cancel_event.is_set(): | ||
| raise CancelledException("Operation cancelled") | ||
|
|
||
| # convert to a string and pass to the python callback | ||
| data_bytes = ctypes.string_at(data_ptr, length) | ||
| data_str = data_bytes.decode('utf-8') | ||
| self._py_callback(data_str) | ||
| return 0 # continue | ||
| except CancelledException as e: | ||
| if self is not None and self.exception is None: | ||
| self.exception = e | ||
| return 1 # cancel |
There was a problem hiding this comment.
self may be referenced before assignment in the except CancelledException (and the generic except Exception) if an exception occurs before self = ctypes.cast(...) succeeds. Initialize self = None before the try, and then guard if self is not None safely in the exception handlers to avoid an UnboundLocalError masking the original failure/cancellation.
| user_cb = progress_callback if progress_callback is not None else lambda _pct: None | ||
| response = self._core_interop.execute_command_with_callback( | ||
| "download_model", request, | ||
| lambda pct_str: progress_callback(float(pct_str)) | ||
| lambda pct_str: user_cb(float(pct_str)), | ||
| cancel_event, | ||
| ) |
There was a problem hiding this comment.
When cancel_event is provided without a progress_callback, this still attempts float(pct_str) for every chunk. If the native layer ever emits non-float chunks (e.g., whitespace-separated tokens, headers, or other status text), this will raise and fail the download unexpectedly. Consider making the callback parsing resilient (e.g., try/except ValueError to ignore non-float tokens, or splitting whitespace and parsing tokens similar to the Rust implementation).
| signal = namesOrCallbackOrSignal; | ||
| } else { | ||
| names = namesOrCallback; | ||
| signal = maybeSignal; |
There was a problem hiding this comment.
The final else branch no longer preserves the old behavior where callers could pass (undefined, progressCallback) (or (undefined, signal)) and still have the second argument recognized. Previously, names would become undefined and progressCallback could still be taken from the second parameter; now progressCallbackOrSignal is ignored entirely in this branch. To avoid a backward-compat regression, handle this branch by interpreting progressCallbackOrSignal as either a progress callback or an AbortSignal when the first argument is undefined/missing.
| signal = maybeSignal; | |
| if (typeof progressCallbackOrSignal === 'function') { | |
| progressCallback = progressCallbackOrSignal; | |
| signal = maybeSignal; | |
| } else if (isAbortSignal(progressCallbackOrSignal)) { | |
| signal = progressCallbackOrSignal; | |
| } else { | |
| signal = maybeSignal; | |
| } |
| let cancelled = state.is_cancelled(); | ||
|
|
||
| // Flush any trailing partial UTF-8 bytes. | ||
| state.flush(); | ||
|
|
||
| if cancelled { | ||
| // Free native response memory before returning the error. | ||
| Self::process_response(response).ok(); | ||
| return Err(FoundryLocalError::CommandExecution { | ||
| reason: "Operation cancelled".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
cancelled is derived from reading the shared AtomicBool after the native call returns. This can race: the flag may be set after the operation successfully completes but before this check runs, causing a spurious "Operation cancelled" error. A more robust approach is to track “cancellation was observed and requested during a callback” inside StreamingCallbackState (e.g., set a cancelled_observed field when the trampoline returns cancel) and only return a cancellation error when cancellation was actually acted upon during streaming.
No description provided.