Repository navigation
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24706
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 1 Unrelated Failure, 1 Unclassified FailureAs of commit af61b8d with merge base 60a8b5f ( NEW FAILURES - The following jobs have failed:
UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:
BROKEN TRUNK - The following job 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
|
3f4257a to
10f0224
Compare
10f0224 to
dbc9a83
Compare
dbc9a83 to
a6f5115
Compare
|
@claude review effort=max |
|
Claude finished @JakeStevens's task in 48m 10s —— View job Claude finished reviewing PR #24706This PR replaces the ESP-IDF component's built-in op profiles with per-project kernel bundles. I recommend Request Changes for two main reasons: a project without a bundle now builds cleanly but registers no kernels (this includes any project still set up for the removed Detailed review: 2 major and 8 minor findings, plus 13 line-level commentsPR Review: #24706SummaryThe PR replaces the component's built-in op profiles (
The rest are minor. All 15 changed files were reviewed. Nothing was built or run, because this job has no ESP-IDF toolchain, so the CMake and link behavior described here comes from reading the code. Paths are under Backward Compatibility
API Design
Testing
Performance
Code Quality
Specific Comments
RecommendationRequest Changes: both major findings turn a configuration mistake into a failure on the device with no clear cause, and both have small fixes. The minor findings can be fixed here or in follow-ups.
This was a review only. No code was changed, and nothing was built or run, because this job has no ESP-IDF toolchain. |
The generated component no longer bakes in op profiles. It vendors every kernel source of the portable, quantized and Cadence generic kernel libraries into a plain static library, ships gen_kernels.py with the libraries' yamls and ExecuTorch's codegen templates under tools/, and registers the kernels of the project's kernel bundle. A project declares its models and bundle with executorch_add_model and executorch_add_kernels from project_include.cmake. At the end of the project's configure, a deferred step checks that the bundle was made for this component, by version and commit, and for exactly the declared models (by SHA-256), rejects delegates the component does not provide yet, and compiles the bundle's registration and prim op selection into the component. A mismatch fails at configure time and prints the gen_kernels.py command that fixes it. A bundle used without declared models is used as is, with a warning that names the models it was made from. Registration no longer needs WHOLE_ARCHIVE. Each registration file is compiled through a small wrapper that defines an extern "C" anchor, and the deferred step adds -u for each one, which reaches the link from a deferred call. Only the bundle's files are anchored, so a project that declares no models and no bundle links nothing from the component: CI checks that such an app is the same size with and without it (169,728 B on ESP32-S3). The kernel library is not even compiled without a bundle. et_model_create references a symbol that only the bundle's registration defines, so an app that runs a model without declaring one fails to link on et_requires_executorch_add_kernels instead of failing every load with missing operators. CONFIG_ET_ENABLE goes away with the profile choice. The deferred step reads the bundle's kernel count from its selected_max_kernel_num.h, which it requires, and sizes the registry with it unless CONFIG_ET_MAX_KERNEL_NUM, now relative to the bundle, sets a larger size for an application that registers its own kernels; configuring one smaller than the bundle is an error. The smoke image on ESP32-S3 is 253,668 B, down from 266,660 B with the smoke profile, because the bundle selects only the prim ops the model uses (none here); with all 28 prims selected it is 266,684 B. CI also builds the smoke consumer for ESP32-C6 (setup.sh gains --targets) with a bundle that adds the ops of the Cadence-quantized wake word model, so the Cadence generic kernels stay compiled and linked. It checks that a re-exported model, a model the bundle was not made from, a bundle made for another component version or commit or missing its kernel count, and models without a bundle each fail at configure time, that a bundle used as is names its models, and that an app running an undeclared model fails to link. Projects that use an op profile, such as CONFIG_ET_OP_PROFILE_WAKEWORD_INT8, drop CONFIG_ET_ENABLE and the profile and move to a kernel bundle made from their model. Authored with Claude Code.
a6f5115 to
af61b8d
Compare
JakeStevens
left a comment
There was a problem hiding this comment.
LGTM. A few non-blocking nits, fine to address here or in a follow-up:
project_include.cmake:358-362:CMAKE_CONFIGURE_DEPENDSlistskernels.jsonand the models but not${_dir}/VERSION. If the component is regenerated in place at the same version from a different commit, the generated CMake files come out byte-identical, so an incrementalidf.py builddoesn't reconfigure and the commit check never runs. That is exactly the case the check was written for. Adding"${_dir}/VERSION"there fixes it. Thecommit-mismatchCI case only covers a freshreconfigure, so it can't catch this.test_espressif_idf_component.sh:213-226: the only emulator run useslarge_registry.defaults(CONFIG_ET_MAX_KERNEL_NUM=64). The default path, where the registry is sized exactly fromselected_max_kernel_num.h, is built (C6) but never executed. A short count would fail registration at boot while CI stays green. Consider running the emulator on the default-sized build.project_include.cmake:13,50-52:executorch_add_backend()is documented as "force-link a delegate", but every call reaches aFATAL_ERROR(lines 77-80 and 259-264). Drop it until a delegate exists, or mark it as reserved.- Once a bundle exists, all ~250 portable, quantized and Cadence kernel sources compile, even for a model with a handful of ops.
--gc-sectionstakes care of flash, but clean builds get slower for users and in CI. It may be worth a line in the README. - In the size loop, the second
consumer ... sizecall overwritesfirmware-${name}.log(the redirect inconsumeruses>), so theempty/empty-baselinebuild logs are lost from the artifacts. - The version, commit and stale "Regenerate it" messages print a bare
python .../gen_kernels.py. In an IDF shell that's IDF's venv, which has noexecutorch, so a copy-paste gives aModuleNotFoundError. The no-bundle message already says which environment to use; the others could say the same.
(Review assisted by Claude Code.)
The generated component no longer bakes in op profiles. It vendors every kernel
source of the portable, quantized and Cadence generic kernel libraries into a
plain static library, ships gen_kernels.py with the libraries' yamls and
ExecuTorch's codegen templates under tools/, and registers the kernels of the
project's kernel bundle. A project declares its models and bundle with
executorch_add_model and executorch_add_kernels from project_include.cmake. At
the end of the project's configure, a deferred step checks that the bundle was
made for this component, by version and commit, and for exactly the declared
models (by SHA-256), rejects delegates the component does not provide yet, and
compiles the bundle's registration and prim op selection into the component. A
mismatch fails at configure time and prints the gen_kernels.py command that
fixes it. A bundle used without declared models is used as is, with a warning
that names the models it was made from.
Registration no longer needs WHOLE_ARCHIVE. Each registration file is compiled
through a small wrapper that defines an extern "C" anchor, and the deferred
step adds -u for each one, which reaches the link from a deferred call. Only
the bundle's files are anchored, so a project that declares no models and no
bundle links nothing from the component: CI checks that such an app is the same
size with and without it (169,728 B on ESP32-S3). The kernel library is not
even compiled without a bundle. et_model_create references a symbol that only
the bundle's registration defines, so an app that runs a model without
declaring one fails to link on et_requires_executorch_add_kernels instead of
failing every load with missing operators. CONFIG_ET_ENABLE goes away with the
profile choice. The deferred step reads the bundle's kernel count from its
selected_max_kernel_num.h, which it requires, and sizes the registry with it
unless CONFIG_ET_MAX_KERNEL_NUM, now relative to the bundle, sets a larger size
for an application that registers its own kernels; configuring one smaller
than the bundle is an error.
The smoke image on ESP32-S3 is 253,668 B, down from 266,660 B with the smoke
profile, because the bundle selects only the prim ops the model uses (none
here); with all 28 prims selected it is 266,684 B. CI also builds the smoke
consumer for ESP32-C6 (setup.sh gains --targets) with a bundle that adds the
ops of the Cadence-quantized wake word model, so the Cadence generic kernels
stay compiled and linked. It checks that a re-exported model, a model the bundle
was not made from, a bundle made for another component version or commit or
missing its kernel count, and models without a bundle each fail at configure
time, that a bundle used as is names its models, and that an app running an
undeclared model fails to link.
Projects that use an op profile, such as CONFIG_ET_OP_PROFILE_WAKEWORD_INT8,
drop CONFIG_ET_ENABLE and the profile and move to a kernel bundle made from
their model.
Authored with Claude Code.