Skip to content

[CoreAI] Parse the delegate manifest at runtime and run host tests in CI - #23387

Open
metascroy wants to merge 3 commits into
mainfrom
coreai-v2/01-export-parser
Open

metascroy wants to merge 3 commits into
mainfrom
coreai-v2/01-export-parser

Conversation

@metascroy

@metascroy metascroy commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Adds the native parser the Core AI runtime uses to read the delegate manifest, and builds and runs the backend's host tests in the Core AI workflow. This is the first of the runtime PRs; it does not add an ExecuTorch delegate yet.

  • Runtime (runtime/coreai_assets.{h,mm}):
    • Parse and validate the manifest for inline source and aot_compiled_inline assets: function, ordered input_names/output_names, the files size map and bundle_digests.
    • Select AOT assets by exact platform and Core AI architecture match, with no fallback.
  • Tests: a Foundation-only coreai_host_test GoogleTest executable (no Swift, no Core AI SDK) for the parser and asset selection. Path and file-map checks are covered because they feed filesystem writes; the manifest is otherwise treated as trusted exporter output.
  • README: the "Runtime" manifest description, AOT architecture selection and "Host Tests".
  • backends/apple/coreai/.clang-format: the root style without its Language: Cpp key, so clang-format also formats the backend's Objective-C headers.
  • Changes outside backends/apple/coreai are CMake and CI only, and nothing changes unless EXECUTORCH_BUILD_COREAI=ON:
    • tools/cmake/preset/default.cmake: add EXECUTORCH_BUILD_COREAI (default OFF).
    • CMakeLists.txt: add the backend subdirectory under that option.
    • .github/workflows/coreai.yml: build backends/apple/coreai/all for macOS 27 with tests on, and run every CTest test the backend registers (--no-tests=error).

Stack: 1 of 9 Core AI runtime PRs, based on main. Next: #23388.

Test plan: built locally for macOS 27.0 (backends/apple/coreai/all, EXECUTORCH_BUILD_TESTS=ON); ctest -N lists coreai_host_test, 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/23387

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

✅ You can merge normally! (3 Unrelated Failures)

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

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@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 3m 9s —— View job


This PR adds the Core AI manifest parser, AOT asset selection, a host GoogleTest target and CI wiring. The parser logic looks correct against the exporter in compiler/preprocess.py. What needs fixing: the README's Host Tests section and one CMake define refer to things this PR doesn't add, the AOT-specific checks have no tests, and the CI path filter misses the files this build now depends on.

Detailed review

Summary

The PR adds parse_manifest/select_assets for inline and aot_compiled_inline manifests, a Foundation-only coreai_host_test, and wires the build into coreai.yml. I didn't find a correctness bug in the parser. I checked it against the Python exporter: hash = main.hash hex, path/archs layout, files/bundle_digests from _asset_metadata, and min_deployment_version spelling. The problems below are in docs, tests and CI.

Code Quality

  • backends/apple/coreai/README.md:47-63 and backends/apple/coreai/CMakeLists.txt:38 describe things this PR doesn't add.
    • The README says the host test is "routing any SDK calls through a fake bridge, so it also runs on macOS 26". This PR has no bridge and no SDK calls, and CI only builds it with CMAKE_OSX_DEPLOYMENT_TARGET=27.0.
    • The README command passes -DEXECUTORCH_ENABLE_PROGRAM_VERIFICATION=ON -DEXECUTORCH_BUILD_EXECUTOR_RUNNER=OFF, which CI doesn't use. It also uses deployment target 26.0, which CI never exercises.
    • COREAI_ASSETS_TESTING=1 is defined but nothing reads it.
    • Either move these to the later stack PR that adds the bridge, or make the README match the CI invocation. Fix this →
  • backends/apple/coreai/runtime/coreai_assets.mm:282-287: the files.count > 0 check in select_assets can never fail.
    • validate_files already requires populated == bundles, so every arch in archs has at least one file.
    • Drop the check, or switch it to an assertion of that invariant.
  • backends/apple/coreai/CMakeLists.txt:7-9 and :29-31: the configure guards fail too broadly.
    • if(NOT APPLE) stops with "Core AI host tests require Apple platforms" even when EXECUTORCH_BUILD_TESTS=OFF.
    • The Darwin check makes EXECUTORCH_BUILD_COREAI=ON + EXECUTORCH_BUILD_TESTS=ON fail the whole configure for iOS cross-builds, when it could just skip the host test.
    • Suggest wording the first message about the backend rather than the tests, and gating the test target with if(EXECUTORCH_BUILD_TESTS AND CMAKE_SYSTEM_NAME STREQUAL "Darwin").
  • backends/apple/coreai/.clang-format duplicates the root .clang-format (244 lines) with only Language: Cpp removed.
    • Nothing keeps the two in sync, so changes to the root style will silently stop applying here.
    • At minimum add a header comment naming the source file. Better, add a lint or CI check that diffs it against the root file.

Infrastructure

  • .github/workflows/coreai.yml:11-13: the pull_request path filter covers only coreai.yml and backends/apple/coreai/**.
    • This job now builds the root CMakeLists.txt, tools/cmake/** (where the option lives) and links executorch from runtime/**.
    • A PR that breaks the Core AI CMake build through those files won't run this job; the break only shows up on main pushes.
    • Add at least CMakeLists.txt and tools/cmake/** to the filter.

Testing

  • backends/apple/coreai/runtime/test/coreai_manifest_test.mm: the AOT-only rejection paths in parse_manifest (coreai_assets.mm:164-196) are untested.
    • The PR says path and file-map checks are covered because they feed filesystem writes, but the archs paths and hash feed the same writes. The checks with no test:
      • missing or empty platform;
      • empty or non-object archs;
      • an arch name containing / or ..;
      • an archs path that isn't <hash>/model.<arch>.aimodelc;
      • a files entry under a bundle not listed in archs;
      • an archs entry with no files;
      • a non-hex or path-like hash (e.g. "../x", for both packagings).
    • CoreAIManifestMetadataTest only parameterizes digest coverage over AOT. Adding these as aot_manifest_dict() mutations would cover the AOT validation surface.

Recommendation

Approve once the README and the unused define are fixed. The test gaps and the CI path filter are worth handling here or in the next stack PR; none of them is a correctness bug in the parser.

Summary:
Add the native parser the Core AI runtime uses to read the delegate manifest,
and build and run the backend's host tests in the Core AI workflow.

- Runtime (`runtime/coreai_assets.{h,mm}`): parse and validate the manifest
  for `inline` source and `aot_compiled_inline` assets, including the ordered
  I/O names, file sizes and bundle digests, and select AOT assets by exact
  platform and Core AI architecture match with no fallback.
- Tests: a Foundation-only `coreai_host_test` GoogleTest executable (no Swift,
  no Core AI SDK) for the parser and asset selection. Path and file-map checks
  are covered because they feed filesystem writes; the manifest is otherwise
  treated as trusted exporter output.
- README: the "Runtime" manifest description, AOT architecture selection and
  "Host Tests".

Changes outside `backends/apple/coreai` are CMake and CI only, and nothing
changes unless `EXECUTORCH_BUILD_COREAI=ON`:
- `tools/cmake/preset/default.cmake`: add `EXECUTORCH_BUILD_COREAI`
  (default OFF).
- `CMakeLists.txt`: add the backend subdirectory under that option. The host
  test is registered inside the backend when `EXECUTORCH_BUILD_TESTS=ON`.
- `.github/workflows/coreai.yml`: build the backend directory for macOS 27 and
  run every CTest test it registers.

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`.
Align the parser-only PR with its actual supported behavior and cover malformed
manifest metadata before later storage and loading work depends on it.

Test Plan:
Built and ran the native host suite on macOS 27: all 24 tests passed.
CMake platform/test gating and workflow formatting-config checks passed.
The Python suite was blocked during collection by the installed PyTorch/torchao
compatibility error.

Authored with assistance from Devmate (OpenAI GPT-6-Astra).
@metascroy

Copy link
Copy Markdown
Contributor Author

@claude review this code

Updated head: 5f68c119164cbe7bf6203fa86ab69e2a63690358. Please reassess the updated code and previous findings. The stack has been rebased and refreshed; review this PR against its current base rather than treating downstream changes as part of this PR.

@claude

claude Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

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


This PR adds the Core AI manifest parser, AOT asset selection, a Foundation-only host test, and CI wiring. Every finding from the previous review is fixed at 5f68c11. What's left is non-blocking: no test checks the values the parser returns, two nil inputs aren't handled, a likely gap in the asset path collision checks, and the .mm files aren't covered by clang-format lint.

Detailed review

Summary

This PR parses and validates inline and aot_compiled_inline manifests, selects AOT bundles by exact platform and architecture, and runs coreai_host_test in coreai.yml.

Previous findings, all resolved:

  • README "fake bridge" and the unused COREAI_ASSETS_TESTING define: removed.
  • The files.count > 0 check in select_assets that could never fail: removed.
  • CMake guards: the test is now gated on EXECUTORCH_BUILD_TESTS AND CMAKE_SYSTEM_NAME STREQUAL "Darwin".
  • .clang-format drift: CI now checks that the copy matches the root style.
  • Path filter: it now covers CMakeLists.txt, tools/cmake/** and runtime/**.
  • AOT test gaps: platform, archs, architecture names, bundle paths, undeclared and empty bundles, and the hash for both packagings are all tested now.

Not run: this runner is Linux, so I could not build or run anything. The diff was taken against base commit 61ec450, because origin/main has no merge base in this shallow checkout. The diff stats match the PR's 11 files.

Code Quality

  • runtime/coreai_assets.mm:128 and :246-252: two nil inputs fail badly at the public entry points.
    • parse_manifest(nil) passes nil to JSONObjectWithData:. Foundation then raises NSInvalidArgumentException, and with -fno-exceptions that kills the process instead of returning InvalidProgram.
    • select_assets(m, arch, nil) passes platform.UTF8String, which is NULL, to %s. Apple's libc happens to print (null), but the behavior is formally undefined. The architecture check right after it already handles nil (:263); the platform check doesn't.
    • Suggest an ET_CHECK_OR_RETURN_ERROR(data != nil, ...) at the top of parse_manifest, and the same nil ? "nil" : ... pattern for platform.
  • runtime/coreai_assets.mm:68-87, :176-191: duplicate and parent-path checks compare strings exactly.
    • Default macOS APFS ignores case and treats different Unicode normalizations of a name as the same file.
    • Example: {"model.aimodel/One": 1, "model.aimodel/one/two": 2} passes the parent check at :80-87. On disk, One and one/ are the same entry.
    • The same applies to architecture keys arch_a and ARCH_A, and to NFC vs. NFD spellings of one name.
    • The PR justifies these checks because they feed filesystem writes, so this is the remaining gap. Low confidence that it matters in practice: an exporter listing a real directory on macOS can't produce such pairs, but one running on Linux can. Comparing decomposedStringWithCanonicalMapping.lowercaseString keys would close it.

Infrastructure

  • backends/apple/coreai/.clang-format and the new CI sync step don't enforce formatting on any .mm file.
    • The CLANGFORMAT linter in .lintrunner.toml:71-74 only includes **/*.h and **/*.cpp.
    • So the copied config only reaches coreai_assets.h and coreai_manifest_fixture.h. The PR's 3 .mm files are never checked against it.
    • The test file already breaks the 80-column limit at coreai_manifest_test.mm:18, 19, 56, 109, 125, 265, 286.
    • Either run clang-format over the .mm files and add a lint pattern for backends/apple/coreai/**/*.mm, or note in the PR that the copy is only meant for editor and IDE formatting.

Testing

  • runtime/test/coreai_manifest_test.mm: no test checks the fields that parse_manifest returns.
    • The "accepts" tests (:17, :73, :98-105) only check .ok(). Nothing in runtime/test/ reads function, inputs, outputs, minimum_version, hash, or the inline path/files.
    • The README calls input_names/output_names order a binding contract, yet a mistake such as assigning dict[@"output_names"] to inputs (coreai_assets.mm:151-152) would pass the whole suite. The same goes for a parse_version regression that stores the wrong numbers.
    • One test that parses manifest_dict() and compares each field against the fixture would cover this. Include a "26.4"-style version check on minimum_version.

Recommendation

Approve. None of these blocks merge. The returned-field test and the two nil guards are small, so they're worth adding in this PR.

Summary:
Reject manifests whose asset names collide on case-insensitive, Unicode-normalizing filesystems before materialization. Handle missing input safely, verify returned metadata and ordering, and enforce Objective-C++ formatting in backend CI.

Test Plan:
The new filesystem-alias regressions failed before the fix. All 32 native parser tests and all 113 CoreAI Python tests passed afterward, covering source/AOT aliases, valid Unicode names, nil input, returned fields, name ordering, and version parsing. Formatting, workflow syntax, and whitespace checks passed.

This branch was previously deployed

1 inactive deployment
cadence — 57fedbaa Deployed Oct 27, 2026 by metascroy via hifi-op-test / hifi4 #34932
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