镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Provide the ESP-NN delegate and esp_nn:: op library in the ESP-IDF component - #24708

Open
rascani wants to merge 1 commit into
esp-idf-auto-kernelsfrom
esp-idf-espnn
Open

rascani wants to merge 1 commit into
esp-idf-auto-kernelsfrom
esp-idf-espnn

Conversation

@rascani

@rascani rascani commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

The component now vendors the ESP-NN delegate runtime
(backends/espressif/espnn_runtime) and the esp_nn:: op library
(backends/espressif/ops), depends on espressif/esp-nn (or a project's own
components/esp-nn), and carries the delegate's Kconfig options in its menu. The
op library is a fourth kernel library for gen_kernels.py, after portable,
quantized and Cadence generic. The delegate is linked only when the kernel
bundle records a model that uses EspNNBackend, or when the project calls
executorch_add_backend(EspNNBackend): a small source in the delegate's library
defines an anchor that references register_espnn_backend, so -u on that anchor
pulls in espnn_backend.cpp and its static registration from a library that is
otherwise never linked. Only espnn_backend.h is public, and it now says when
the generated component links it. The kernel library, too, is linked only
when the bundle has kernel libraries or prim ops, so a delegate-only bundle
never builds it, and a project that declares only the backend also defines the
symbol et_model needs to link.

The smoke consumer now embeds any number of models, each with the test case
that split_bundled_program.py extracts from its bundled program, and compares
every output with the reference on the device. CI runs four images on the
emulator. An ESP32-S3 image runs the smoke model on portable kernels, a
quantized mul on esp_nn::quantized_mul_int8 and the conv-act-pool-linear ESP-NN
delegate program together. An ESP32-C6 image runs the first two and must not
contain EspNNBackend. Two more ESP32-S3 images run the delegate alone, one from
a bundle with no kernel libraries, which must not build the kernel library,
and one from executorch_add_backend with no bundle, whose registry must hold
no kernels. A project with its own components/esp-nn must build that one and
not the managed copy. The empty-app size check still passes with esp-nn
compiled in. export_smoke_model.py loses --pte-output, which nothing uses now.

Authored with Claude Code.

@pytorch-bot

pytorch-bot Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 2 New Failures, 2 Pending, 1 Unrelated Failure

As of commit 415d3ea with merge base 60a8b5f (image):

NEW FAILURES - The following jobs have failed:

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.

@rascani
rascani added this pull request to stack #24713 November 5, 2026 23:48
@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 Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 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.

@rascani
rascani requested a review from JakeStevens November 6, 2026 20:46
@rascani
rascani marked this pull request as ready for review November 6, 2026 20:47
@rascani
rascani requested a review from larryliu0820 as a code owner November 6, 2026 20:47
@rascani

rascani commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@claude review effort=max

@claude

claude Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Claude finished @rascani's task in 36m 46s —— View job


Request Changes: the CI check that the ESP32-C6 build doesn't link EspNNBackend can never fail as written; the rest of the findings are minor.

Detailed review
  • Gather PR context and diff against origin/esp-idf-auto-kernels
  • Run three independent reviewers
  • Consolidate findings
  • Fact-check each finding against the code
  • Post review

PR Review: #24708

Summary

This PR vendors the ESP-NN delegate and the esp_nn:: op library into the generated ESP-IDF component. It links the delegate when a kernel bundle records EspNNBackend or the project calls executorch_add_backend(EspNNBackend), and adds an esp_nn:: program and a delegate program to the smoke CI. The ESP32-C6 check that the op library doesn't pull in the delegate can't fail, so CI never verifies the separation this PR depends on. The other findings are minor:

  • a new esp-nn dependency that isn't documented
  • two more CI checks that pass silently
  • a misleading public header
  • a needless kernel build for delegate-only bundles
  • small issues in the test harness and the build script

Backward Compatibility

  • idf_component.yml.in:15, CMakeLists.txt:33: every consumer on every chip now needs espressif/esp-nn, including projects that use neither the delegate nor esp_nn:: ops. Before this PR the only dependency was idf. Projects that build without the component manager (IDF_COMPONENT_MANAGER=0) or without registry access now fail at configure unless they add components/esp-nn themselves. The README mentions neither the dependency nor the components/esp-nn alternative (CMakeLists.txt:39-46).

API Design

  • backends/espressif/espnn_runtime/include/espnn/espnn_backend.h:39-44: the component now ships this header publicly (build_idf_component.sh:287).

    • Its doc describes the standalone espnn_runtime component: "registration also happens from a static initializer when the component is linked whole-archive".
    • In the generated component, the delegate is a plain static library. It is linked only when a bundle records EspNNBackend or the project calls executorch_add_backend(EspNNBackend).
    • So an app that follows the header and calls register_espnn_backend(), delegate_stats() or reset_delegate_stats() without declaring the backend fails to link with an undefined reference.

    Document that requirement for this component, or keep the header private.

Testing

  • (major) .ci/scripts/test_espressif_idf_component.sh:259-263: this check passes even when EspNNBackend is present.

    • grep -q exits at the first match, and nm dies with SIGPIPE on its next write.
    • With pipefail, the pipeline then returns 141, so the if takes the pass branch.
    • The check fires only if nm has finished writing before grep exits, which means the match must be near the end of the symbol table.

    Fix: write the nm output to a file and grep that. Replacing -q with > /dev/null doesn't help, because GNU grep then also stops at the first match.

  • .ci/scripts/test_espressif_idf_component.sh:244-254: the local esp-nn case runs only reconfigure and prints OK without checking which esp-nn the build used.

    • The component's manifest still requests espressif/esp-nn.
    • CMakeLists.txt:42 prefers espressif__esp-nn whenever it is in BUILD_COMPONENTS.
    • So if the component manager also adds the managed copy, the project's own copy is silently ignored and the case still passes.

    Fix: assert that espressif__esp-nn is absent from build_components in the build's project_description.json. Low confidence: this depends on how the component manager resolves the two names.

  • .ci/scripts/test_espressif_idf_component.sh:276-281: if awk finds no registered_kernels_data (after a rename, or if nm -C prints it differently), ${registry:-0} makes the check pass. Fail when registry is empty.

  • Untested paths:

    • The unknown-delegate error (project_include.cmake:63-68) has no test. An expect_configure_error case with an unknown backend would cover it.
    • A bundle combined with executorch_add_backend (the union at project_include.cmake:291-302) is never exercised, because the smoke project never declares both at once.
    • main.c:103 now passes "forward", so nothing covers the documented NULL = first method behavior (et_model.h:24, :111).

Performance

  • project_include.cmake:361 links ${_lib}_kernels for every bundle. That includes delegate-only bundles such as delegate_kernels, which CI checks has no kernel libraries (test_espressif_idf_component.sh:89-96).

    • As a result, delegate-only builds compile every portable, quantized, Cadence and esp_nn:: kernel source.
    • This contradicts the comment at CMakeLists.txt:36-40: "links each only when it is needed, so other projects never build them".

    Fix: link _kernels only when the bundle has libraries or prim_ops. Prim-op registration still needs et_view and et_copy_index from _kernels.

Code Quality

  • CMakeLists.txt:20: src/espnn_anchor.cpp is compiled into the main component library for every project.

    • It references register_espnn_backend from ${_et_espnn}, which is linked only on demand.
    • So linking the component whole-archive fails with an undefined reference.
    • The kernel and prim-op anchors, by contrast, are generated and added with target_sources only when needed (project_include.cmake:325-346).

    Fix: add this anchor the same way in __executorch_link_backends, or move it into ET_ESPNN_SRCS.

  • examples/espressif/idf_component_smoke/main/main.c:77-94: the output comparison differs from BundleIO's elem_is_close and tensors_are_close (devtools/bundled_program/bundled_program.cpp:104-124, :207-234).

    • Matching NaNs and same-sign infinities are reported as mismatches, because fabsf(a - b) is NaN.
    • Double and half outputs are compared with memcmp, ignoring the tolerance.
    • So an output that verify_method_outputs accepts can fail here.

    Fix: mirror elem_is_close.

  • examples/espressif/build_idf_component.sh:437: the ESP-NN Kconfig is spliced in with sed -n '/^menu /,/^endmenu/p'.

    • Anything outside a menu (a top-level config, if or source) is silently dropped.
    • A nested menu whose endmenu is unindented ends the range early.
    • If the file changes shape, the result is empty and there is no error.

    Fix: copy the file into the component and rsource it, or at least fail when the extraction is empty.

  • examples/espressif/build_idf_component.sh:219-226: the delegate sources bypass copy_srcs, which every other vendored source goes through (:209-218). They also don't follow the layout that the section comment at :192 describes ("Output mirrors upstream paths under src/executorch/"). Pipe a find of backends/espressif/espnn_runtime/src into copy_srcs like the kernel sources, or update the comment.

Specific Comments

  • .ci/scripts/test_espressif_idf_component.sh:45: this was the only caller of export_smoke_model.py --pte-output (export_smoke_model.py:30, :49-52). The option is now dead; remove it.
  • .ci/scripts/test_espressif_idf_component.sh:54-55: test_mul.py:130-131 uses torch.finfo(torch.float32).eps (1.1920929e-07), and 1.2e-7 rounds it up. Say so in the comment, or pass the exact value.
  • examples/espressif/idf_component_template/idf_component.yml.in:15: ^1.4 restates the ^1.4.0 in backends/espressif/espnn_runtime/idf_component.yml, so a version bump has to touch both files.

Recommendation

Request Changes

The ESP32-C6 check (test_espressif_idf_component.sh:259-263) is the only test that the esp_nn:: op library doesn't drag in the delegate, and as written it can't fail. Everything else is minor.

…mponent

The component now vendors the ESP-NN delegate runtime
(backends/espressif/espnn_runtime) and the esp_nn:: op library
(backends/espressif/ops), depends on espressif/esp-nn (or a project's own
components/esp-nn), and carries the delegate's Kconfig options in its menu. The
op library is a fourth kernel library for gen_kernels.py, after portable,
quantized and Cadence generic. The delegate is linked only when the kernel
bundle records a model that uses EspNNBackend, or when the project calls
executorch_add_backend(EspNNBackend): a small source in the delegate's library
defines an anchor that references register_espnn_backend, so -u on that anchor
pulls in espnn_backend.cpp and its static registration from a library that is
otherwise never linked. Only espnn_backend.h is public, and it now says when
the generated component links it. The kernel library, too, is linked only
when the bundle has kernel libraries or prim ops, so a delegate-only bundle
never builds it, and a project that declares only the backend also defines the
symbol et_model needs to link.

The smoke consumer now embeds any number of models, each with the test case
that split_bundled_program.py extracts from its bundled program, and compares
every output with the reference on the device. CI runs four images on the
emulator. An ESP32-S3 image runs the smoke model on portable kernels, a
quantized mul on esp_nn::quantized_mul_int8 and the conv-act-pool-linear ESP-NN
delegate program together. An ESP32-C6 image runs the first two and must not
contain EspNNBackend. Two more ESP32-S3 images run the delegate alone, one from
a bundle with no kernel libraries, which must not build the kernel library,
and one from executorch_add_backend with no bundle, whose registry must hold
no kernels. A project with its own components/esp-nn must build that one and
not the managed copy. The empty-app size check still passes with esp-nn
compiled in. export_smoke_model.py loses --pte-output, which nothing uses now.

Authored with Claude Code.

This branch was previously deployed

1 inactive deployment
cadence — 415d3ea3 Deployed Nov 7, 2026 by rascani via hifi-op-test / hifi4 #37199
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.

1 participant