Skip to content

[CoreAI] Backend-local PTE inspection - #23394

Open
metascroy wants to merge 3 commits into
coreai-v2/05b-sdk-delegatefrom
coreai-v2/06-pte-inspect
Open

metascroy wants to merge 3 commits into
coreai-v2/05b-sdk-delegatefrom
coreai-v2/06-pte-inspect

Conversation

@metascroy

@metascroy metascroy commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Adds a private reader that extracts 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.
  • Always runs the flatbuffer structural verifier, the supported-schema-version check and the runtime's semantic validator, even with 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.
  • Inline buffers borrow the retained program image. Only external segments referenced by CoreAIBackend delegates are loaded, keeping their original FreeableBuffer callbacks; unrelated 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}) 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.
  • README: "Backend-local PTE Inspection".

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 -N lists coreai_host_test, coreai_swift_bridge_test and coreai_runtime_smoke, which the Core AI workflow runs on the macOS 27 runner. lintrunner is clean.

@pytorch-bot

pytorch-bot Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23394

Note: Links to docs will display an error until the docs builds have been completed.

❌ 41 Cancelled Jobs, 10 Pending, 12 Unclassified Failures

As of commit e87ff80 with merge base 61ec450 (image):

UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:

CANCELLED JOBS - The following jobs were cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 3, 2026
@metascroy
metascroy force-pushed the coreai-v2/05b-sdk-delegate branch from c14c6ea to 836b175 Compare October 4, 2026 19:48
@metascroy
metascroy force-pushed the coreai-v2/06-pte-inspect branch from 7094d5e to f69015e Compare October 4, 2026 19:48
@metascroy
metascroy force-pushed the coreai-v2/05b-sdk-delegate branch from 836b175 to a6f16ff Compare October 5, 2026 03:14
@metascroy
metascroy force-pushed the coreai-v2/06-pte-inspect branch from f69015e to 011fb30 Compare October 5, 2026 03:14
@metascroy

Copy link
Copy Markdown
Contributor Author

@claude review this PR

@claude

claude Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Claude finished @metascroy's task in 2m 44s —— View job


PR Review: #23394

I found no correctness bugs. The main issue is that inspect_coreai_pte adds several validation checks that Program::load doesn't have and that no test exercises; the rest are small cleanups.

Detailed review

Summary

This PR adds inspect_coreai_pte, which pulls CoreAIBackend processed buffers out of a PTE without building a Program. I checked it against Program::load / Program::LoadSegment. Header parsing, segment offsets, the order buffers are freed in (processed before program_storage) and error propagation are all correct. The concerns are:

  • validation code that goes beyond the "trusted exporter output" stance and has no tests
  • gaps in the tests
  • a few naming and housekeeping items

Code Quality

  • Validation goes further than both Program::load and the stated trust model. backends/apple/coreai/runtime/coreai_pte.cpp:107-121 reads the header a second time from the retained image and compares every field. That only catches a loader that returns different bytes for the same offset, which the header contract already rules out ("loader and its data must remain stable"). Several other checks have no counterpart in Program::load:

    • unique method names
    • non-null non_const_buffer_sizes, inputs and outputs
    • segment_base_offset >= program_size
    • non-empty inline processed data
    • buffer->size() != size after cpu_data already checked < size

    Two problems follow:

    1. None of these checks is tested (see Testing).
    2. The reader can reject a PTE that Program::load accepts. The cache maintenance that relies on it would then not see that program's Core AI models.

    Program::load also returns InvalidArgument for a misaligned image, while this code returns InvalidProgram (:103-106).

    Given CLAUDE.md's "when uncertain: choose simpler", I'd match Program::load + LoadSegment semantics and drop the extra checks. Otherwise, document and test each one as a deliberate requirement.

  • Dead null checks. In a verified buffer, Get() on a flatbuffer vector of tables never returns null. That makes delegate == nullptr (:146), buffers->Get(index) == nullptr (:162) and segments->Get(index) == nullptr (:174) unreachable.

  • Unused include. #include <cstring> in coreai_pte.cpp isn't used.

  • Names don't match. The suite is coreai_program_data_test.mm / CoreAIProgramDataTest, but it tests coreai_pte.{h,cpp}. The fixture types (CacheDelegateSpec, CacheMethodSpec) are named for cache use rather than PTE building. Renaming to coreai_pte_test.mm / CoreAIPteTest would make the suite easier to find.

  • README describes internals of a private API. The "Backend-local PTE Inspection" section covers the 64-byte prefix read, the load order and buffer ownership of a function the PR itself calls private. The ownership and lifetime contract belongs in the coreai_pte.h doc comment, where callers will look. I'd keep the README to one or two sentences.

Testing

  • Validation branches are untested. Nothing exercises:

    • header bounds: program_size > file_size, segment_base_offset out of range, segment_data_size overflow
    • an out-of-range processed index for a CoreAIBackend delegate, inline or segment
    • a missing processed reference on a CoreAIBackend delegate
    • SEGMENT location without an extended header
    • duplicate or null method names
    • a misaligned program image

    The fixture already supports index_override and missing_processed. Both are only used on delegates whose backend ID doesn't match, so the inspector skips them before the checks run. Pointing them at CoreAIBackend delegates would cover most of the list cheaply. If you take the simplification above, the list gets much shorter.

  • Unused fixture fields. fail_backend_index (coreai_pte_fixture.h) is never set by a test. LoadFault has only None/Error, so a bool would do. The DedupVtables(false) comment talks about "malformed-field tests", but only the version test changes a field.

  • Weak assertion. ReleasesEarlierBuffersOnLateFailure uses EXPECT_GE(pte.backend_loads, 1). With fault_call = 3 the count is always exactly 1, so EXPECT_EQ would catch a regression that GE misses.

Recommendation

Approve. Nothing blocks merging. Before landing, I'd recommend either trimming the validation back to Program::load parity or adding tests for the extra checks.

@metascroy

Copy link
Copy Markdown
Contributor Author

@claude review this code

Updated head: ba2439e5cc22621102df1df10d37deff55c93af0. The stack and parent PR have been refreshed; please review against the current base.

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 Program::method_meta, although not by Program::load; accepting it here does not establish method executability.

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 InvalidArgument to match runtime loading. The fault enum is retained because it now represents distinct error, short, oversized, and misaligned responses rather than a boolean failure.

@claude

claude Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Claude finished @metascroy's task in 3m 45s —— View job


PR Review: #23394 (head ba2439e)

I re-reviewed against the refreshed base and found no correctness bugs. The previous findings are resolved, and I agree with your corrections. What remains is minor: one stated guarantee has no test, a few places differ from the runtime, and two comments are out of date.

Detailed review

Summary

inspect_coreai_pte reads the CoreAIBackend processed buffers out of a PTE without constructing a Program. I checked the header bounds, segment offsets, ownership and destruction order, and load-error propagation against Program::load / LoadSegment, and they are correct. Three independent reviewers read all seven files. Every item below was re-checked against the code. Nothing was built or run.

Code Quality

  • A malformed non-CoreAI delegate makes the whole inspection fail (runtime/coreai_pte.cpp:124-126).
    • What happens: the delegate->id() == nullptr check runs before the backend filter. validate_program doesn't check delegate ids, so this line is the only thing that rejects them.
    • Consequence: if any method has a delegate with no id, even one belonging to another backend, inspection returns InvalidProgram. Cache maintenance then can't see that program's Core AI models.
    • Why it's inconsistent: the runtime only fails when that particular method is loaded. That conflicts with the rule "only check references this reader dereferences".
    • Suggested fix: a delegate with a null id can't be CoreAIBackend, so treat it as unselected and continue.
  • Some error codes and header checks differ from Program::load/LoadSegment, even though alignment now matches.
    • Error codes: the runtime returns NotFound in three cases where this code returns InvalidProgram (coreai_pte.cpp:140, :151-152, locked in by tests at coreai_pte_test.mm:305-327):
      • an inline index out of range (program.cpp:570)
      • a segment reference with no header or segment_base_offset == 0 (:605)
      • a segment index out of range (:616)
    • Header checks: coreai_pte.cpp:77-81 checks segment_base_offset against program_size/file_size even when segment_data_size == 0. Program::load skips that check for the pre-1.0 BC case (program.cpp:99-103), so a legacy-header PTE with inline-only Core AI data is loaded by the runtime but rejected here. Real exporter output is unlikely to hit this.
    • Suggested fix: match the runtime in both places, or note in coreai_pte.h that the reader uses its own, stricter error contract.
  • The ownership comment is misleading (runtime/coreai_pte.h:21). "Borrowed inline data and segment descriptors must die before the program" suggests segment buffers borrow the image, but they own their data. The rule that actually matters is that processed is declared after program_storage, so it is destroyed first and the inline borrows stay valid. Saying that the member order is load-bearing would stop someone from reordering the fields.
  • The backend ID string is duplicated (coreai_pte.cpp:127 and coreai_backend.mm:398). If the registered name changes, inspection won't fail; it will silently return an empty result. A shared constant would prevent that.
  • README Host Tests paragraph is outdated (README.md:278-283). It still lists only manifest, assets, storage, acquisition and bookmarks, but coreai_host_test now includes the PTE inspection suite too.

Testing

  • validate_program never fails in any test (coreai_pte.cpp:113-114). The PR says the semantic validator always runs, but RejectsMalformedFlatbufferAndUnsupportedVersion (coreai_pte_test.mm:415-431) only covers the verifier (bad identifier) and the version check. If you deleted the validate_program(...) != Error::Ok clause, every test would still pass. A buffer that passes the verifier but fails semantic validation (for example clear_field(plan, fb::ExecutionPlan::VT_VALUES) with the existing helper) would cover it.
  • Two rejection branches have no test:
    • a null delegate id (:124). The fixture already supports backend == nullptr, but no test uses it.
    • an unknown DataLocation (:187). The FlatBuffer verifier doesn't range-check enums, so this branch is the only guard.
  • All tests use synthetic PTEs (coreai_pte_fixture.mm). The fixture writes the extended header, 16-byte segment padding and segment_data_size its own way. exir/_serialize/_program.py uses different alignment defaults. If the fixture and the reader misread the layout the same way, the tests still pass. One test on a real exported PTE with a CoreAIBackend delegate, even a tiny checked-in one, would cover this. Low priority.

Recommendation

Approve. Nothing here blocks the merge. Of the items above, the validate_program test and the null-id continue are the most worthwhile follow-ups.

@metascroy
metascroy force-pushed the coreai-v2/06-pte-inspect branch from fb0cea1 to 95295f1 Compare October 28, 2026 17:38
@metascroy
metascroy force-pushed the coreai-v2/06-pte-inspect branch from 95295f1 to f2259cb Compare October 28, 2026 19:45
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.

This branch is being deployed

1 in progress deployment
cadence — e87ff809 Deployed Oct 29, 2026 by metascroy via hifi1s-build / hifi1s #35199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants