[rascani]

The component's README is its page on the ESP Component Registry, and it
opened with how the tree is generated and versioned, which tells a
newcomer nothing about what ExecuTorch is or where to start. It now
introduces ExecuTorch and how a model goes from torch.export to a .pte
that the runtime runs from flash, lists what the component carries, and
gives the command that creates the hello_executorch project from the
registry. The usage notes become steps in the order a project takes
them, with an export example for the ESP-NN delegate, run against the
current package, and the et_model example from its header. How the tree
is generated and versioned moves to the end.

Authored with Claude Code.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⏳ No Failures, 1 Pending

As of commit 03ab16a with merge base 0fc43f6 ():
💚 Looks good so far! There are no failures yet. 💚

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

[github-actions]

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]

@claude review

[claude]

Claude finished @rascani's task in 2m 52s —— View job

---

The rewrite holds up against the code, but it drops the esp_nn:: op library the component still ships, and a few claims go further than the code supports (memory planning, hello_executorch availability, RC create-project).

Detailed review

PR Review: #24861

Summary

The PR reorganizes the ESP-IDF component's registry README around what ExecuTorch is, a quick start, and step-by-step usage. Three reviewers checked every changed line against build_idf_component.sh, et_model.h, backends/espressif, and the publish workflow. One finding is a real omission. The rest are minor accuracy gaps. The Python export example, the C API names, the CMake/Kconfig names, the version scheme, the @ET_COMPONENT_VERSION@ substitution and the links all match the code.

Code Quality

• README.md:38-39: the esp_nn:: op library is gone (all 3 reviewers). The old text listed four kernel libraries: "portable, quantized, the Cadence backend's generic kernels and the esp_nn:: op library". The new bullet lists only three, and the op library is not mentioned anywhere else (git diff shows the old line removed, -and the \esp_nn::` op library). The component still ships it: build_idf_component.sh:39-40,500addsesp_nn backends/espressif/ops/operators.yaml, and CI exercises it (.ci/scripts/test_espressif_idf_component.sh). The "Export a model" section also covers only the delegate path. It never mentions to_espnn_edge(..., use_operator_library=True) (backends/espressif/lowering.py), which emits esp_nn::` operators instead of delegates. Please restore the library to the list, and add a sentence on the operator-library path.

• README.md:22-24: the memory-planning claim goes too far. The README says lowering "plans where every tensor lives in memory, so the device runs the model in arenas whose sizes are known before it starts". et_model.h:62-71 says otherwise:

  • The method pool holds "non-planned tensors" and defaults to a guessed 64 KiB.
  • With no temp pool (the default), "the runtime then mallocs scratch through the PAL during execute".

  Suggest "plans where the model's activations live", and drop the claim that all sizes are known up front.

• README.md:61-63 (and :47): a self-generated copy may not contain examples/hello_executorch. build_idf_component.sh:290-292 sets EXAMPLE=false and only prints a warning under --allow-dev-stamp when the installed executorch doesn't match the checkout. That is the common local-dev case, where the README sends the user to "build examples/hello_executorch in place". A short qualifier would fix it, for example "if the generator included it (it needs an executorch package built from the same checkout)".

• README.md:55: the create-project command fails on RC pages (lower confidence, not run). publish-espressif-idf.yml:82-83,108-110 publishes -rc.N versions to the staging registry only, and the README is rendered per version. An RC's page therefore shows pytorch/executorch==X.Y.Z-rc.N:hello_executorch. That command resolves against the production registry by default, where the version doesn't exist. A note that RC users must point idf.py at the staging registry would help. The == dependency snippet has the same gap, but that predates this PR.

• README.md:179-195: the "Run the model" snippet.

  • model_pte/model_pte_size appear from nowhere. The pointer to hello_executorch/main/CMakeLists.txt doesn't say that the example uses a hand-written .S with .balign 16 + .incbin. Plain EMBED_FILES doesn't guarantee that alignment, and et_model_create rejects misaligned data (src/et_model.cpp:233-237). One sentence naming the technique would stop readers reaching for EMBED_FILES.
  • The snippet ignores the bool returns of et_model_set_input and et_model_execute (et_model.h:160,166). It is copied from the header comment, but readers will copy it from the registry page. Checking them as the snippet already does for et_model_load_method costs one line.

Recommendation

Approve once the esp_nn:: op library is back in the "What the component provides" list. The remaining items are minor wording fixes that can go into this PR or a follow-up.

[rascani]

@claude review

[claude]

Claude finished @rascani's task in 3m 33s —— View job

---

The amended README fixes most of what the last review raised, but the esp_nn:: op library is still missing from the component's contents, and the note on when the generator includes hello_executorch doesn't match build_idf_component.sh.

Detailed review

PR Review: #24861

Summary

This PR rewrites the ESP-IDF component's registry README as an intro to ExecuTorch, a quick start, and usage steps in order. Three independent reviewers checked it against the code, and I fact-checked their findings. The Python export example, the et_model API, ET_COMPONENT_SMOKE: PASS, the .balign 16/.incbin note, the version scheme and the @...@ placeholders all match the code. Two minor accuracy problems remain, plus a few smaller notes.

Code Quality

• README.md:39-40: the esp_nn:: op library is still missing (all 3 reviewers). The kernel-library bullet lists portable, quantized and Cadence generic only. The component still ships the ESP-NN operator library:

  • build_idf_component.sh:40 names it in the generator's header comment, and build_idf_component.sh:503 ships it ("esp_nn backends/espressif/ops/operators.yaml").
  • to_espnn_edge(..., use_operator_library=True) (backends/espressif/lowering.py) produces .ptes that need it.

  "Export a model" also describes only the delegate path. Please add the library to the bullet, and add one sentence on use_operator_library=True to the export section. Fix this →

• README.md:65-68: the rule for when the generator includes hello_executorch is wrong (2 reviewers, confirmed). The README says the generator includes it "when it runs with the executorch package built from the same commit". The code differs in two ways:

  • versions_match (tools/gen_kernels.py:75-85) also accepts the X.Y.Z / X.Y.Z+cpu release wheel for a release or ~N respin.
  • When nothing matches, the generator leaves the example out only under --allow-dev-stamp. Every other stamp exits with an error (build_idf_component.sh:293-299).

  Suggested wording: "unless it was a --allow-dev-stamp build without an executorch package built from that commit". The "What the component provides" bullet at line 48 lists examples/hello_executorch with no such qualifier.

• README.md:62-65: the RC registry variable needs to stay set for the build too (2 reviewers, low confidence, not run). hello_executorch/main/idf_component.yml.in has no registry_url, and the registry drops its override_path. So idf.py build resolves ==X.Y.Z-rc.N again and needs IDF_COMPONENT_REGISTRY_URL as well. If a reader sets it inline only for create-project-from-example, the build then can't find the RC. Saying "export it in the shell" would avoid this, as would pointing to the registry_url note at lines 81-82.

• README.md:196-201: the C snippet's error branches fall through (minor). After a failure it still calls et_model_output_data. It also never checks whether that call returned NULL (et_model.h:168-171). The snippet mirrors the header comment, but readers will copy it from the registry page, so it should return from the error branches.

• README.md:121,127: undefined placeholders in the export example (nit). MyModel and calibration_inputs are undefined, so pasting the block raises a NameError. report is assigned but never explained, and it is the list of CPU-fallback ops users will want to check. A short comment on each would help.

Recommendation

Approve once the esp_nn:: op library is back in the list and the generator caveat is corrected. The other items are optional wording fixes.

[rascani]

@claude review - Please note that I'm not adding the esp_nn:: op library documentation because its not ready.

[claude]

Claude finished @rascani's task in 3m 21s —— View job

---

The README has no blockers left, and I left the esp_nn:: op library out as you asked. Two minor accuracy gaps remain: the steps never show how model_pte gets into the firmware, and dev versions published with the default settings live only on the staging registry, like RCs.

Detailed review

PR Review: #24861

Summary

This PR rewrites the ESP-IDF component's registry README as an intro to ExecuTorch, a quick start, and usage steps in order. Three independent reviewers checked it against the code, and I fact-checked what they found. As you asked, the esp_nn:: op library is not flagged. Only minor accuracy and usability gaps remain.

Code Quality

• README.md:63-66, 83-84: the staging-only caveat covers release candidates, but dev versions need it too. A nightly-branch dispatch sets staging=true production=true (publish-espressif-idf.yml:76-77). Then line 105, [[ "${REGISTRY}" == production ]] || production=false, turns production off unless the run asked for it, and the registry input defaults to staging (:24). So a -dev version published with the default settings exists only on staging. On its page, the create-project-from-example "pytorch/executorch==X.Y.Z-dev.…" command and the == dependency fail to resolve unless the reader points at staging. Suggest "A release candidate, and a dev version unless it was also published to production, is on the staging registry…".
• README.md:153-157, 194, 210-214: the steps never put model_pte/model_pte_size into the firmware (all 3 reviewers). The "Declare" snippet registers only main.c. executorch_add_model declares the model to the bundle check but embeds nothing (project_include.cmake). A reader who follows the steps as written gets undefined model_pte/model_pte_size at link time.
  • The pointer to hello_executorch/main/CMakeLists.txt helps only partly. Its models.S builds a smoke_models table (name, pte, pte_end, io, lines 20-36), not these symbols, and gets the size as pte_end - pte.
  • Fix: add a minimal .S (.balign 16 / model_pte: / .incbin / model_pte_end:) with model_pte_size = model_pte_end - model_pte, plus the SRCS entry. Or say plainly that executorch_add_model doesn't embed the model.
• README.md:215-216: "scratch arenas are … allocated with heap_caps" is wrong for the default. With temp_arena_size = 0 (the default) there is no scratch pool, and "the runtime then mallocs scratch through the PAL during execute" (et_model.h:68-71). The default 64 KiB method pool (et_model.h:62-64) is also worth naming, since it's the first value users tune.
• README.md:83-84: registry_url can't be added to the short-form dependency shown just above it. pytorch/executorch: "==X" has to become a mapping with version: and registry_url:. A reader who appends registry_url: under the string ends up with an invalid manifest. A three-line YAML example would prevent this.
• README.md:143: python <this directory>/tools/gen_kernels.py. On the registry page, "this directory" has no meaning. A registry user's copy is at managed_components/pytorch__executorch/tools/gen_kernels.py. The text is carried over, but the new readers make it worse.
• README.md:41-45: the claim that the delegate is linked only when a model uses it was dropped (2 reviewers). The old text said the component "links the delegate only when one of them uses it". Now only the kernel half survives (lines 182-184), so readers can't tell whether EspNNBackend costs flash in a project that doesn't use it.

Recommendation

Approve. None of these block the PR. The dev-version registry caveat and the missing embed step are the two worth fixing here.