Repository navigation
Conversation
c14c6ea to
836b175
Compare
7094d5e to
f69015e
Compare
836b175 to
a6f16ff
Compare
f69015e to
011fb30
Compare
|
@claude review this PR |
|
Claude finished @metascroy's task in 2m 44s —— View job PR Review: #23394I found no correctness bugs. The main issue is that Detailed reviewSummaryThis PR adds
Code Quality
Testing
RecommendationApprove. Nothing blocks merging. Before landing, I'd recommend either trimming the validation back to |
011fb30 to
ba2439e
Compare
a6f16ff to
ed11289
Compare
|
@claude review this code Updated head: Context on the previous findings: unrelated method-metadata validation, unique-name checks, and the second header comparison were removed. One correction to the earlier review: required method metadata is checked by We intentionally retained FlatBuffer verification, overflow/range checks, checks for references actually dereferenced by this reader, and the exact returned-buffer-size guarantee. The exact-size check is not redundant with a minimum-size check: it rejects oversized buffers that would otherwise be returned intact. Those cases now have focused coverage, alongside malformed CoreAI references, bounds, alignment, and release-on-failure tests. Misalignment returns |
|
Claude finished @metascroy's task in 3m 45s —— View job PR Review: #23394 (head
|
fb0cea1 to
95295f1
Compare
95295f1 to
f2259cb
Compare
Summary:
Add a private reader that extracts the Core AI processed buffers from a PTE
without constructing a `Program` or `Method`, so cache maintenance can find a
program's Core AI models without loading it.
- `runtime/coreai_pte.{h,cpp}`: `inspect_coreai_pte(DataLoader&)` returns the
`CoreAIBackend` processed buffers in method/delegate order.
- It always runs the flatbuffer structural verifier, the supported schema
version check and the runtime's existing semantic validator, even when
`ET_ENABLE_PROGRAM_VERIFICATION=0`. It also checks method metadata, unique
method names, exact backend IDs, data ranges, alignment and CPU
accessibility.
- It reuses the existing extended-header parser and verifier rather than
duplicating program parsing. No shared runtime code changes.
- Inline buffers borrow the retained program image. Only external segments
referenced by `CoreAIBackend` delegates are loaded, and they keep their
original `FreeableBuffer` callbacks. Unrelated external payloads and named
data are not loaded.
- `CoreAIPteData` owns the program image and buffers. It is move-only, and the
loader must outlive it.
- Tests: a synthetic PTE builder (`runtime/test/coreai_pte_fixture.{h,mm}`).
The suite covers inline and segmented delegates, method-order routing,
exact backend-ID matching, header formats, load-error propagation and
buffer release on late failure. The PTE is treated as trusted exporter
output, so malformed-PTE robustness is limited to the verifier and version
checks.
- README: "Backend-local PTE Inspection".
No changes outside `backends/apple/coreai`.
Test Plan:
```
# Local: configure and build only
cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release \
-DCMAKE_OSX_DEPLOYMENT_TARGET=27.0 -DEXECUTORCH_BUILD_COREAI=ON \
-DEXECUTORCH_BUILD_TESTS=ON -DEXECUTORCH_BUILD_EXTENSION_DATA_LOADER=ON
cmake --build build --target backends/apple/coreai/all
# CI (macOS 27 runner, .github/workflows/coreai.yml)
ctest --test-dir build/backends/apple/coreai --output-on-failure --no-tests=error
```
CTest runs `coreai_host_test` (adding the PTE inspection suite),
`coreai_swift_bridge_test` and `coreai_runtime_smoke`.
# Conflicts:
# backends/apple/coreai/CMakeLists.txt
# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# interactive rebase in progress; onto 8b1d879
# Last command done (1 command done):
# pick b9624f7 # [CoreAI] Backend-local PTE inspection
# Next command to do (1 remaining command):
# pick ba2439e # [CoreAI] Simplify PTE inspection validation and cover failure paths
# You are currently rebasing branch 'coreai-v2/06-pte-inspect' on '8b1d879b14'.
#
# Changes to be committed:
# modified: backends/apple/coreai/CMakeLists.txt
# modified: backends/apple/coreai/README.md
# new file: backends/apple/coreai/runtime/coreai_pte.cpp
# new file: backends/apple/coreai/runtime/coreai_pte.h
# new file: backends/apple/coreai/runtime/test/coreai_program_data_test.mm
# new file: backends/apple/coreai/runtime/test/coreai_pte_fixture.h
# new file: backends/apple/coreai/runtime/test/coreai_pte_fixture.mm
#
# Changes not staged for commit:
# modified: backends/mlx/third-party/mlx (modified content)
#
Summary: PTE inspection should not reject programs for metadata unrelated to extracting delegate payloads. Remove those requirements and redundant checks while preserving bounds and buffer-ownership guarantees, and verify both compatibility and malformed-input handling. Test Plan: All four Core AI CTest suites passed on macOS 27. New native tests confirmed acceptance of missing method metadata and duplicate method names alongside normal program loading, and acceptance of empty processed buffers. Malformed references, invalid header and segment ranges, misaligned images, and short or oversized loads were rejected with owned buffers released. The full Core AI Python suite passed: 113 tests.
Skip unselected delegates with missing IDs, match runtime missing-reference errors and legacy headers, share the backend identifier, and clarify ownership. Cover semantic validation, unknown locations, safe legacy bounds, and inspection of a real exported PTE. Test Plan: CMake build and all five CTest suites passed (183 native tests, 18 Swift tests, real exported-PTE comparison, portable/AOT source lifetime, registration smoke). Full Python suite: 124 passed. Formatting, Ruff and whitespace checks passed. Generic Buck owner validation still fails without diagnostics.
f2259cb to
e87ff80
Compare
fcc39d9 to
d07e206
Compare
Adds a private reader that extracts Core AI processed buffers from a PTE without constructing a
ProgramorMethod, so cache maintenance can find a program's Core AI models without loading it (runtime/coreai_pte.{h,cpp}).inspect_coreai_pte(DataLoader&)returns theCoreAIBackendprocessed buffers in method/delegate order.ET_ENABLE_PROGRAM_VERIFICATION=0; also checks method metadata, unique method names, exact backend IDs, data ranges, alignment and CPU accessibility. Reuses the existing extended-header parser; no shared runtime code changes.CoreAIBackenddelegates are loaded, keeping their originalFreeableBuffercallbacks; unrelated payloads and named data are not loaded.CoreAIPteDataowns the program image and buffers; it is move-only and the loader must outlive it.runtime/test/coreai_pte_fixture.{h,mm}) and a suite covering inline and segmented delegates, method-order routing, exact backend-ID matching, header formats, load-error propagation and buffer release on late failure. The PTE is treated as trusted exporter output, so malformed-PTE coverage is limited to the verifier and version checks.No changes outside
backends/apple/coreai.Stack: 8 of 9, based on #23393. Review only this PR's commit. Next: #23395.
Test plan: built locally for macOS 27.0;
ctest -Nlistscoreai_host_test,coreai_swift_bridge_testandcoreai_runtime_smoke, which the Core AI workflow runs on the macOS 27 runner.lintrunneris clean.