Add named manual kernel registration API

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.

[goutamadwant]

Summary

Adds an opt-in library-name parameter for manual kernel registration codegen so generated RegisterKernels.{h,cpp} can expose a library-specific registration API such as register_portable_ops_lib_kernels().

The default manual registration API remains register_all_kernels() when no library name is provided. Named registration requires a valid C++ identifier, preventing distinct library names from collapsing to the same generated symbol.

CMake exposes MANUAL_REGISTRATION explicitly on both code generation and operator-library creation, avoiding hidden cross-call state while keeping generated filenames and target sources aligned. The Buck macro supports the same optional name and rejects a name supplied without manual registration.

Fixes#11221.

Test plan

• PYTHONPATH=.. python3 -m unittest codegen.test.test_executorch_gen codegen.test.test_executorch_signatures codegen.test.test_executorch_types codegen.test.test_executorch_unboxing codegen.test.test_selective_build
• python3 -m py_compile codegen/gen.py codegen/test/test_executorch_gen.py
• Changed-file lintrunner checks
• Configured and installed the ExecuTorch CMake targets locally
• Configured and built the named_manual_registration_test target with TEST_NAMED_MANUAL_REGISTRATION=ON
• Ran named_manual_registration_test, which registers two generated libraries and verifies both operators are present
• git diff --check

cc @larryliu0820@JacobSzwejbka@lucylq

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⚠️ 17 Awaiting Approval

As of commit 1b978a7 with merge base ed65b12 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build Aarch64 Linux Wheels (gh)
• Build documentation (gh)
• Build Linux Wheels (gh)
• Build macOS Wheels (gh)
• Build Presets (gh)
• Build Windows Wheels (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

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

[goutamadwant]

@pytorchbot label "release notes: api"

[nil-is-all]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

[goutamadwant]

Hi @goutamadwant, thanks for the PR! Could you address the errors in the lintrunner logs? Should be simple fixes

Sure will take a look @nil-is-all

[goutamadwant]

@nil-is-all Addressed the lintrunner formatting issues and pushed the update. I reran the changed-file lintrunner check and the focused codegen unit tests locally. Let me know if there are any suggestions. thanks!

[digantdesai]

@claude review this PR

[claude]

Claude finished @digantdesai's task in 4m 34s —— View job

---

PR Review: Named manual kernel registration API

I reviewed the diff against origin/main. Overall this is a clean, well-scoped, backward-compatible change with good Python-level test coverage. Details below.

What's good

• Backward compatible.get_manual_registration_function_name returns register_all_kernels when no lib name is given, and the templates default to that via the DEFAULT_MANUAL_REGISTRATION_FUNCTION_NAME env value, so existing consumers are unaffected.
• Validation is sensible. Rejecting --manual-registration-lib-name without --manual-registration, and enforcing a valid C++ identifier, both fail fast with clear messages surfaced through parser.error.
• Good unit tests.TestManualRegistrationFunctionName and TestManualRegistrationTemplates cover the default, named, and both error paths, plus actual template rendering for .h and .cpp.
• Buck + CMake plumbing is threaded through consistently, and the write_sharded(..., base_env=...) approach is the right way to inject a non-sharded value into the aggregate RegisterKernelsEverything.cpp.

Issues / suggestions

1. CMake couples the C++ function name to the CMake target name (codegen/gen.py validation + tools/cmake/Codegen.cmake:210-212)

list(APPEND _gen_command --manual-registration
--manual-registration-lib-name=${GEN_LIB_NAME})

GEN_LIB_NAME is a CMake target name, which is not guaranteed to be a valid C++ identifier (targets legitimately contain -, ., ::). If a caller enables MANUAL_REGISTRATION on such a target, codegen will parser.error("... must be a valid C++ identifier") and hard-fail the build. Worth either sanitizing the name before passing it (e.g. replace non-[A-Za-z0-9_] with _) or documenting the constraint next to the MANUAL_REGISTRATION option. Fix this →

2. The CMake MANUAL_REGISTRATION path has no in-tree caller and is therefore untested.
Grepping the repo, MANUAL_REGISTRATION is only referenced inside tools/cmake/Codegen.cmake itself — no CMakeLists.txt passes it to generate_bindings_for_kernels/gen_operators_lib. Combined with the author's note that CMake couldn't be run locally, the entire CMake branch (including the new RegisterKernelsEverything.cpp / RegisterKernels.h source list) is effectively unexercised. Two suggestions:

• Confirm the sharded aggregate output filename really is RegisterKernelsEverything.cpp for the manual path (the Python test relies on this, so it should be correct — but the CMake add_custom_command OUTPUT must match exactly or the build will error on a missing output).
• Consider wiring one existing lib (or a test lib) through the CMake path in a follow-up so CI covers it.

3. Very generic CLI aliases (codegen/gen.py:997-1004).
--lib-name / --lib_name are broad names for what is specifically the manual-registration lib name. Since codegen already has a LIB_NAME concept elsewhere, these short aliases could invite confusion. Consider dropping them and keeping just --manual-registration-lib-name (+ underscore variant).

4. Minor: redundant anchors in the regex.
re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") combined with .fullmatch() double-anchors; fullmatch already anchors both ends. Harmless, just noise.

Test verification

I attempted to run codegen.test.test_executorch_gen locally but the sandbox blocked the Python invocation, so I could not independently execute the suite. The tests read correctly and the template/base_env wiring is sound on inspection. The CI Lint/pull jobs (currently awaiting approval) should confirm.

Verdict

No blocking correctness bugs in the Python codegen. The main thing I'd want addressed before merge is #1 (CMake target-name → C++ identifier coupling), since it can turn into a confusing build failure. #2 is a coverage gap worth a follow-up. The rest are minor/nits.
· branch 11221-named-manual-kernel-registration

[goutamadwant]

@claude review this again

Addressed the CMake target-name concern by sanitizing the manual registration library name before it is embedded in the generated C++ symbol. For example, target/library names with characters like -, ., or :: now produce a valid registration function name instead of causing codegen to fail.

I also updated the focused codegen tests and reran the changed-file lintrunner check locally. The new workflow runs are currently waiting for approval.

[shoumikhin]

Thanks for picking up #11221 — opt-in flag with the default preserved is the right shape, and doing Buck + CMake together is appreciated. Approving. A couple of non-blocking things worth a look:

1. CMake↔codegen filename parity. The MANUAL_REGISTRATION branch swaps the generated sources to RegisterKernelsEverything.cpp / RegisterKernels.h (vs RegisterCodegenUnboxedKernelsEverything.cpp). Worth confirming codegen actually emits those names in that mode and the Buck path stays consistent — a mismatch here would be a silent build break.

2. CMake passes the raw LIB_NAME as the identifier (--manual-registration-lib-name=${GEN_LIB_NAME}). The validator requires ^[A-Za-z_][A-Za-z0-9_]*$, so a target whose name has a - or . would hard-error codegen. Either sanitize -/.→_ or document that manual-registration targets need identifier-safe names.

Nit: the flag has four spellings (--manual-registration-lib-name / --manual_registration_lib_name / --lib-name / --lib_name) — one canonical plus the underscore alias is plenty.

Nice validation + codegen tests otherwise; this is the right fix for the multi-lib registration collision.

[goutamadwant]

Thanks for the review @shoumikhin I removed the generic --lib-name and --lib_name aliases, keeping --manual-registration-lib-name and its underscore variant.

For your other two points:

• The manual registration tests generate and read RegisterKernelsEverything.cpp and RegisterKernels.h, matching the filenames used by CMake.
• CMake can continue passing the target name directly because codegen sanitizes it before embedding it in the C++ registration symbol. The sanitization is covered by the portable-ops.lib::debug test case.

I also reran the focused manual registration tests and the changed-file lintrunner checks. Let me know if any other suggestions. thanks!

[shoumikhin]

Inline findings from a holistic design and implementation review.

[shoumikhin]

This sanitization is lossy, so distinct libraries can still generate the same global symbol. For example, foo-bar, foo.bar, and foo_bar all become register_foo_bar_kernels(), causing duplicate definitions when those libraries are linked together. Could we either reject non-identifier names or use a collision-resistant encoding/hash suffix? Avoiding registration-symbol collisions is the primary purpose of this API.

[goutamadwant]

@shoumikhin I removed the lossy sanitization. --manual-registration-lib-name now requires a valid C++ identifier, so names such as foo-bar and foo.bar fail instead of collapsing to the same generated symbol. Valid identifier names also remain unchanged and I added tests for invalid punctuation and leading digits.

[shoumikhin]

MANUAL_REGISTRATION must now be repeated independently in both generate_bindings_for_kernels() and gen_operators_lib(). If a caller enables it on only one call, codegen and target_sources() expect different filenames (RegisterKernelsEverything.cpp versus RegisterCodegenUnboxedKernelsEverything.cpp), resulting in a missing-source build failure. Could we derive/store this mode per LIB_NAME, combine the configuration, or at least fail at configure time when the two calls disagree?

[goutamadwant]

Updated this so generate_bindings_for_kernels() records the manual-registration mode for each LIB_NAME, and gen_operators_lib() derives the mode from that configuration. Callers should no longer repeat MANUAL_REGISTRATION in both calls.

[shoumikhin]

If manual_registration_lib_name is supplied without manual_registration = True, this macro silently ignores the name. Direct codegen rejects the same combination with --manual-registration-lib-name requires --manual-registration. Could we add a Starlark fail() here so the public Buck API has the same validation contract?

[goutamadwant]

Added a Starlark fail() when manual_registration_lib_name is supplied without manual_registration = True, matching the validation contract of the Python codegen entry point.

[shoumikhin]

Could we add an end-to-end CMake test for this branch? No in-tree CMake caller currently enables MANUAL_REGISTRATION, and the added Python tests only verify template rendering. A useful regression test would build two manually registered libraries, include both generated headers, call both named functions, and verify both kernel sets register. That would also catch mismatches between custom-command outputs and target_sources().

[goutamadwant]

@shoumikhin added an end-to-end CMake regression to the existing portable custom-ops workflow. It builds two independently named manual-registration libraries, includes both generated headers, calls both generated registration functions, and verifies that both operators are present in the runtime registry.
Let me know!

[goutamadwant]

@shoumikhin addressed your comments / suggestions. let me know if this looks good. thanks!

[shoumikhin]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).

2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

[goutamadwant]

Two things I'd like addressed on this.

1. The help text and the bzl docstring describe behavior the code does not implement. Both the --manual-registration-lib-name help in codegen/gen.py and the executorch_generated_lib docstring in shim_et/xplat/executorch/codegen/codegen.bzl say "Characters that are not valid in a C++ identifier are converted to underscores." The code does the opposite: get_manual_registration_function_name raises ValueError on such a name, and generate_bindings_for_kernels in tools/cmake/Codegen.cmake hits FATAL_ERROR. So a name like my-ops.lib is a hard build failure, not a sanitized register_my_ops_lib_kernels. Please update both strings to state that the lib name must already be a valid C++ identifier (I'd keep the strict validation and just correct the docs rather than add sanitization).
2. In tools/cmake/Codegen.cmake, gen_operators_lib recovers the manual-registration flag by reading a GLOBAL property keyed on SHA256(LIB_NAME) that generate_bindings_for_kernels set earlier. This is an implicit contract between two separate function calls with an ordering hazard: if gen_operators_lib runs before generate_bindings_for_kernels for the same LIB_NAME, the property is empty, it silently selects RegisterCodegenUnboxedKernelsEverything.cpp (which is never generated in manual mode), and the build fails at compile time with no indication of the cause. Please pass this explicitly instead: add a MANUAL_REGISTRATION option to gen_operators_lib, symmetric with generate_bindings_for_kernels, and set it from the caller. That removes the hidden global state and the ordering trap. If you prefer to keep the property channel, at least drop the SHA256 hashing, since LIB_NAME is already validated as a C++ identifier and is a legal property suffix, so the hash only hides the key from cmake --trace.

@shoumikhin Addressed both these points in the latest push. below are the highlights.

• Updated the CLI help and Starlark documentation to describe the strict C++ identifier requirement.
• Removed the global property and passed MANUAL_REGISTRATION explicitly to gen_operators_lib.
• Updated the CMake caller to pass the option to both codegen functions.

test pass locally. Let me know if there are any other suggestions/comments.

[shoumikhin]

The naming mechanism itself looks right, and the two things I asked for last time are done. My remaining concern is that this renames the symbol but does not quite finish making a second kernel library usable. Four things:

1. A consumer that links the library still cannot include the generated header, because gen_operators_lib never sets an include directory on the target. The new example works around this with its own target_include_directories.
2. Two of these libraries cannot both be installed, because the generated header goes into PUBLIC_HEADER and CMake flattens those to basenames at install time.
3. docs/source/using-executorch-faqs.md:77 still tells users there must be only one generated operator library per target, which this PR's own example contradicts. Nothing in docs/ changes here.
4. Two named libraries still abort at runtime if their operator sets overlap. That is a real constraint on the feature and it is neither documented nor tested.

Also, no build or test workflow has run on this head yet, they are all waiting for approval, so the new end to end test has not executed anywhere.

[shoumikhin]

gen_operators_lib never calls target_include_directories on ${GEN_LIB_NAME}, so a consumer that links this library cannot include the generated RegisterKernels.h that the PR now publishes. That is why the new example has to add ${CMAKE_CURRENT_BINARY_DIR} to its own include path by hand. The prim ops helper in this same file does it the other way at line 208, and one target_include_directories(${GEN_LIB_NAME} INTERFACE $<BUILD_INTERFACE:${_out_dir}>) here would let consumers just link the target.

[shoumikhin]

This is the consistency check I asked about earlier and I do not think it landed. If a caller passes MANUAL_REGISTRATION to only one of the two functions, the failure is Cannot find source file: .../RegisterKernelsEverything.cpp followed by No SOURCES given to target, and neither message mentions the option, so the user has no way to connect the error to the mistake. A get_source_file_property(<var> ${_out_dir}/<expected>.cpp GENERATED) check here with a FATAL_ERROR that names MANUAL_REGISTRATION would make it obvious.

[shoumikhin]

Adding RegisterKernels.h to PUBLIC_HEADER breaks the two library case at install time. CMake flattens PUBLIC_HEADER entries to basenames, and the in-tree pattern for installing one of these targets sends them all to one flat directory (see kernels/portable/CMakeLists.txt:101), so two manual registration libraries write the same filename and one is silently dropped. I reproduced it with two targets and one destination: the install log prints "Installing" then "Up-to-date" for the same path and only the first library's declaration survives. Installing under a per library subdirectory such as <includedir>/executorch/<lib_name>/ would fix it.

[shoumikhin]

GEN_LIB_NAME is unquoted here, so when LIB_NAME is not passed at all CMake compares the literal string GEN_LIB_NAME against the regex, which matches, and the check silently passes. Verified with cmake -P on 3.31.8: unset is accepted, empty string is correctly rejected. Quoting it as NOT "${GEN_LIB_NAME}" MATCHES ... closes it.

[shoumikhin]

This was inserted between manual_registration and use_default_aten_ops_lib, which shifts the thirteen parameters after it. Every in-tree caller uses keyword arguments so nothing here breaks, but the macro is public and does not require keyword-only calls, so an out-of-tree positional caller would silently bind use_default_aten_ops_lib to the new name. Appending it at the end of the signature avoids that at no cost.

[shoumikhin]

Two inputs get through and regenerate the exact symbol this option exists to avoid. A name of all returns register_all_kernels, identical to the default, so an unnamed library and one named all collide at link time. An empty string short circuits before both the identifier check and the "requires manual registration" check, so an unset build variable expands to nothing and quietly falls back to the default. Testing if manual_registration_lib_name is None for omission and rejecting all covers both.