Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18824
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 5 New Failures, 3 Unrelated FailuresAs of commit 1015912 with merge base 5e8a0df ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Enables ExecuTorch flatbuffer/program verification by default (including release builds) and makes Verification::InternalConsistency a strict requirement when requested, rather than silently falling back to minimal checks.
Changes:
- Default CMake preset
EXECUTORCH_ENABLE_PROGRAM_VERIFICATIONtoON(release builds now include verification unless explicitly disabled). - Change
Program::load()(and related Module/Python entrypoints) default verification level toVerification::InternalConsistency. - When verification code is compiled out, requesting
InternalConsistencynow returnsError::NotSupportedinstead of logging and falling back.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/cmake/preset/default.cmake | Defaults program verification build option to ON. |
| CMakeLists.txt | Updates documentation/comments around the verification build flag behavior and size impact. |
| runtime/executor/targets.bzl | Updates Buck/Bazel-side comments describing verification size and behavior when disabled. |
| runtime/executor/program.h | Switches default verification argument to InternalConsistency. |
| runtime/executor/program.cpp | Enforces NotSupported when InternalConsistency is requested but compiled out; keeps minimal checks only for Minimal. |
| extension/pybindings/pybindings.cpp | Updates Python bindings’ default verification argument to InternalConsistency. |
| extension/module/module.h | Updates Module API default verification argument to InternalConsistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Error, | ||
| "InternalConsistency verification requested but not available. " | ||
| "Build with ET_ENABLE_PROGRAM_VERIFICATION=1 or " | ||
| "use Verification::Minimal to skip verification."); |
There was a problem hiding this comment.
The error text says "use Verification::Minimal to skip verification", but Minimal still performs basic checks (e.g., root table offset bounds check below). Consider rewording to avoid implying verification is entirely skipped (e.g., "use Verification::Minimal for basic verification" / "reduced verification").
| "use Verification::Minimal to skip verification."); | |
| "use Verification::Minimal for basic verification."); |
| ET_NODISCARD static Result<Program> load( | ||
| DataLoader* loader, | ||
| Verification verification = Verification::Minimal); | ||
| Verification verification = Verification::InternalConsistency); | ||
|
|
There was a problem hiding this comment.
With the default argument now set to Verification::InternalConsistency, building with ET_ENABLE_PROGRAM_VERIFICATION=0 (or equivalent build option) will make Program::load(loader) fail with Error::NotSupported unless callers explicitly pass Verification::Minimal. If the goal is that space-constrained builds can "opt out" purely via the build flag, consider making the default conditional on ET_ENABLE_PROGRAM_VERIFICATION (or introducing a "Default" mode) or documenting this behavior prominently.
[edit] on ci this adds closer to 10-12kB, and puts us way above 50kb :(
linux: echo 'Fail 56448 > 45000'
gcc: echo 'Fail 60296 > 48500'
Seems like this only adds 8kb, so let's try to enable it. It's logging that adds 20-30kb.
Verification::InternalConsistencyeven in release builds.Verification::InternalConsistencyis requested but not available, error out.Test Plan
Check CI size test