Coreml patch#1696
Conversation
7b94225 to
b4a0a94
Compare
b4a0a94 to
6bf954b
Compare
ac788ec to
8fe41d4
Compare
ogad-tether
left a comment
There was a problem hiding this comment.
The CoreML external-data patch direction looks good, but I want a couple of follow-ups before approving:
- Please make the scope of the registry baseline bump explicit. The new baseline is several commits ahead of the old one, so this PR currently brings in more than just the ONNX Runtime CoreML fix.
- Please confirm CI after the workflow fix is actually exercised from the base branch, because the current failing
sanity-checksrun still reflects the oldpull_request_targetworkflow behavior.
Once those are addressed or clarified, I’m happy to take another look.
Tier-based Approval Status |
…th fix Patched ONNX Runtime 1.24.2 CoreML EP to fix !model_path.empty() crash when loading models with external data files on macOS/iOS. Backport of microsoft/onnxruntime#28062. - Pin onnxruntime deps to >= 1.24.2#5 (registry port-version with fix) - Bump package version to 0.14.1 - Pin workflow action refs to commit SHA (sparse-checkout fix) Made-with: Cursor
|
please see manual run: https://github.com/tetherto/qvac/actions/runs/24911782474 |
|
Non-blocking note: the PR description still says this change bumps the registry baseline and picks up onnxruntime port-version 4. The final diff now pins onnxruntime directly in vcpkg.json to 1.24.2#5 and leaves vcpkg-configuration.json unchanged, so it would be good to update the description to match the shipped implementation. |
ogad-tether
left a comment
There was a problem hiding this comment.
LGTM. I re-checked the current diff and do not see any blocking issues in the package change itself.
The earlier baseline-scope concern appears resolved: this branch no longer changes vcpkg-configuration.json, and CI is actually resolving onnxruntime 1.24.2#5 from the manifest pin. I verified that in the successful prebuild logs, and the package/prebuild matrix is green across Linux, macOS, Windows, iOS, and Android.
The only remaining red signal is sanity-checks, and that failure is in the YAML-formatting step rather than in dependency resolution or the ONNX package build/test flow.
Preview deployments for qvac-docs-staging ⚡️
Commit: Deployment ID: Static site name: |
|
/review |
🎯 What problem does this PR solve?
CoreML EP in ORT 1.24.2 crashes with !model_path.empty() was false when loading ONNX models with external data files (.onnx_data / .onnx.data) on macOS/iOS
useGPU: true silently falls back to CPU — users think they're running on GPU but they're not
Root cause: TensorProtoWithExternalDataToTensorProto passes the full model file path to ReadExternalDataForTensor, which expects a directory (microsoft/onnxruntime#28005)
📝 How does it solve it?
Pins onnxruntime dependency to >= 1.24.2#5 in vcpkg.json, which includes the backported one-line fix (microsoft/onnxruntime#28062): model_path → model_path.parent_path() in tensorprotoutils.cc
Bumps @qvac/onnx from 0.14.0 → 0.14.1
Pins workflow action refs to commit SHA to fix sparse-checkout issue in on-pr-qvac-lib-infer-onnx.yml
🧪 How was it tested?
Reproduced the !model_path.empty() error on macOS ARM64 with Parakeet TDT encoder (encoder-model.onnx + 2.3 GB .onnx.data) and TTS Chatterbox models using useGPU: true
Verified locally that onnxruntime 1.24.2#5 builds and patches apply cleanly on arm64-osx
Upstream fix includes a regression test (CoreMLExecutionProviderTest.ExternalDataInitializer) that creates a model with external data and loads it via CoreML EP from a file path