Skip to content

Install: stop pulling every submodule when a required one is missing - #24523

Open
JakeStevens wants to merge 1 commit into
pytorch:mainfrom
JakeStevens:minimal-submodule-update
Open

Install: stop pulling every submodule when a required one is missing#24523
JakeStevens wants to merge 1 commit into
pytorch:mainfrom
JakeStevens:minimal-submodule-update

Conversation

@JakeStevens

@JakeStevens JakeStevens commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

install_executorch.py runs a bare git submodule update --init --recursive whenever a required submodule is missing, which fetches all ~24 submodules, including llama.cpp, Vulkan, googletest and FACTO, none of which the default build uses. This PR updates only the missing required submodule paths instead.

The bulk update was hiding gaps in REQUIRED_SUBMODULES: several submodules the default build needs were only ever fetched as a side effect. flatcc and json are built unconditionally by third-party/CMakeLists.txt. pocketfft is included by the optimized kernels' FFT ops, which the pybind preset enables on every platform. All three are now required. tools/cmake/preset/pybind.cmake also enables MLX on Apple Silicon when Xcode's Metal compiler is present, so backends/mlx/third-party/mlx is required on macOS arm64. llama.cpp is marked shallow = true, as mlx already is.

Testing

On a fresh clone of this branch with no submodules initialized, in a new Python 3.12 conda env on an M-series Mac, I ran ./install_executorch.sh:

  • The install script fetched 17 of the 24 submodules: the required ones plus mlx. llama.cpp, the three Vulkan submodules, googletest, FACTO and ios-cmake stayed uninitialized.
  • The full install completed and the pybindings import, with 245 registered ops including fft_r2c, and with CoreML and XNNPACK registered.

Two local caveats, neither caused by this PR. First, the pinned TORCHAO_NIGHTLY_VERSION (0.19.0.dev20260907) has been pruned from the nightly index, so I bumped it to 0.19.0.dev20260913 in the scratch clone. Second, this Mac's Xcode 26 reports a metal compiler but doesn't have the Metal Toolchain component downloaded, so the MLX build fails. It failed the same way before this PR, which also fetched mlx. The successful run used CMAKE_ARGS=-DEXECUTORCH_BUILD_MLX=OFF. Before that, a configure of the pybind preset with MLX on succeeded once mlx was fetched. Linux was not tested.

Authored with Claude Code.

@pytorch-bot

pytorch-bot Bot commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 101 New Failures, 3 Unrelated Failures

As of commit cc9789d with merge base 459d534 (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@JakeStevens
JakeStevens force-pushed the minimal-submodule-update branch 2 times, most recently from 1ab1f55 to dc18dcc Compare November 4, 2026 16:49
@JakeStevens

JakeStevens commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude review effort:max

@claude

claude Bot commented Nov 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @JakeStevens's task in 3m 12s —— View job

Only fetching the required submodules breaks the default ./install_executorch.sh on a fresh clone made without --recurse-submodules: json and pocketfft (and mlx on Apple Silicon) are built by default but are not in REQUIRED_SUBMODULES. Before this PR they were fetched only by the bulk update.

Detailed review

PR Review: #24523

Summary

The PR limits check_and_update_submodules() to the missing required submodules, adds flatcc to that list, and marks llama.cpp shallow. flatcc was not the only default-build dependency that came in through the bulk update. At least two others did too, and dropping them is a regression for anyone whose clone did not fetch submodules. CI won't catch this: the workflows check out submodules themselves (submodules: recursive, .ci/scripts/wheel/pre_build_script.sh), so check_and_update_submodules() finds nothing missing there. The author's own test only checked which submodules got initialized and did not configure a full build.

Code Quality

  • install_executorch.py:94-109: third-party/json is required but not listed (blocker).
    • third-party/CMakeLists.txt:9 runs add_subdirectory(json) unconditionally. This is the same reason the PR gives for adding flatcc.
    • CoreML, which tools/cmake/preset/pybind.cmake turns on for Linux and macOS, also links nlohmann_json.
    • On a fresh clone, third-party/json stays empty and CMake configure fails.
    • Fix: add "json": "CMakeLists.txt". Among top-level .gitmodules paths the substring matches only third-party/json.
  • install_executorch.py:94-109: third-party/pocketfft is required but not listed (blocker).
    • tools/cmake/preset/pybind.cmake:23 sets EXECUTORCH_BUILD_KERNELS_OPTIMIZED ON on every platform.
    • kernels/optimized/CMakeLists.txt:67 adds third-party/pocketfft to the include path.
    • kernels/optimized/cpu/fft_utils.h:12 does #include <pocketfft_hdronly.h>. op_fft_r2c.cpp and op_fft_c2r.cpp use it, and both are in the optimized kernel sources (build_variables.bzl:273-274).
    • The default install now fails at compile time with a missing-header error that doesn't mention the submodule.
    • The PR description lists "optimized kernels needing pocketfft" as opt-in. In the pybind preset they are on by default.
    • Fix: add "pocketfft": "pocketfft_hdronly.h". pocketfft has no CMakeLists.txt.
  • install_executorch.py:146-151: MLX is on by default on Apple Silicon but its submodule is no longer fetched (major).
    • pybind.cmake:62-75 turns on EXECUTORCH_BUILD_MLX whenever xcrun -sdk macosx --find metal succeeds, which is true on any Mac with Xcode.
    • backends/mlx/CMakeLists.txt:116-120 stops with FATAL_ERROR "MLX submodule not initialized".
    • So a fresh clone plus the default install on an M-series Mac used to work and now fails at configure time. The error message is clear, but this contradicts the claim that the default build doesn't use mlx.
    • Two ways to fix it:
      • Fetch backends/mlx/third-party/mlx in the install script when running on Darwin arm64.
      • Or have pybind.cmake downgrade EXECUTORCH_BUILD_MLX to OFF with a warning when the submodule is missing. That mirrors how setup.py:2795-2820 already skips Vulkan when its submodules are absent.

Fix this →

Recommendation

Request Changes

Add json and pocketfft to REQUIRED_SUBMODULES and handle the MLX default on macOS arm64. Then run the default ./install_executorch.sh once on a clone with no submodules, so you know the narrowed list covers what the build needs.

@JakeStevens
Install: stop pulling every submodule when a required one is missing
cc9789d 
check_and_update_submodules() ran a bare `git submodule update --init
--recursive` whenever a required submodule was absent, which fetched all
~24 submodules including llama.cpp, Vulkan, and googletest, even though
the default build uses none of them. Update only the missing required
paths instead.

flatcc, json, and pocketfft are needed by the default build but were only
ever fetched as a side effect of the bulk update, so add them to
REQUIRED_SUBMODULES. The pybind preset enables MLX on Apple Silicon, so
its submodule is required there too. Mark llama.cpp shallow like mlx to
trim its clone.

This PR was authored with the assistance of Claude Code.
@JakeStevens
JakeStevens force-pushed the minimal-submodule-update branch from dc18dcc to cc9789d Compare November 11, 2026 02:36
@JakeStevens

JakeStevens commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude review effort:max

@claude

claude Bot commented Nov 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @JakeStevens's task in 4m 28s —— View job

The updated REQUIRED_SUBMODULES covers the default build, but on a clone without submodules, installs that turn on Vulkan, VGF or tests used to get those submodules from the bulk fetch and no longer do: Vulkan now drops out of the wheel with no warning, and the documented CMAKE_ARGS="-DEXECUTORCH_BUILD_VULKAN=ON" ./install_executorch.sh command fails at configure.

Detailed review

PR Review: #24523

Summary

When a required submodule is missing, check_and_update_submodules() now fetches only the required submodules. flatcc, json and pocketfft are now required, and so is mlx on macOS arm64. llama.cpp is marked shallow.

The default build looks covered on Linux and macOS arm64. The problem is everything outside the default: the flows that turn on Vulkan, VGF or tests were working only because the bulk fetch pulled their submodules in. Three independent reviewers flagged this regression, and I checked each finding below against the code.

Backward Compatibility

  • install_executorch.py:156-158: opt-in backends and tests no longer get their submodules (major). Nothing in get_required_submodule_paths() looks at CMAKE_ARGS or the environment, so anything outside the default build that needs a submodule now breaks on a clone without submodules:

    • Vulkan goes missing with no warning. setup.py:2795-2818 turns Vulkan on by default (default=True) when glslc is available (install_utils.is_vulkan_available()) and all three Vulkan submodules are present. Before, the bulk fetch brought in volk, Vulkan-Headers and VulkanMemoryAllocator, so a Linux user with the Vulkan SDK got a Vulkan-enabled wheel. Now the presence check fails and the wheel is built without Vulkan, with no message.
    • The documented Vulkan command fails. docs/source/using-executorch-building-from-source.md:118 documents CMAKE_ARGS="-DEXECUTORCH_BUILD_VULKAN=ON" ./install_executorch.sh. The script prints "All required submodules are present", and then CMake stops at the FATAL_ERROR in backends/vulkan/CMakeLists.txt:47.
    • VGF fails the same way. With EXECUTORCH_PYBIND_ENABLE_VGF=ON (tools/cmake/preset/pybind.cmake:33), configure hits the FATAL_ERROR in backends/arm/CMakeLists.txt:192, which asks for Vulkan-Headers and volk.
    • C++ tests fail. Running test/run_oss_cpp_tests.sh after a bare install needs googletest (third-party/CMakeLists.txt:20), which is no longer fetched.

    The CMake errors at least say which submodules to fetch. The Vulkan case gives no sign at all. Two ways to fix it:

    • Add paths to the required set based on the requested options, the same way the PR already adds mlx. Add the Vulkan paths when is_vulkan_available() is true or EXECUTORCH_BUILD_VULKAN is set in CMAKE_ARGS or the env, and add Vulkan-Headers and volk when EXECUTORCH_PYBIND_ENABLE_VGF=ON.
    • Or, at minimum, log which optional submodules were skipped and the command to fetch them, and update the docs line above.

    Fix this →

Testing

  • No CI job runs the new code path. Every CI workflow fetches submodules before running the install (.ci/scripts/utils.sh:25, .ci/scripts/wheel/pre_build_script.sh, _unittest.yml), and .ci/scripts/tests/test_cu134_dependencies.py mocks check_and_update_submodules(). Green CI therefore says nothing about this change.
    • The only manual test was on one Mac, with MLX turned off.
    • Linux x86_64 and aarch64 have not been run from a clone without submodules. The TorchAO kernels are on for Linux aarch64.
    • Please run ./install_executorch.sh on Linux from a clone without submodules once before merging.

Code Quality

  • install_executorch.py:126-128: the mlx rule is broader than what CMake actually builds (minor).
    • The comment says the preset "enables MLX by default on Apple Silicon". pybind.cmake:67-82 enables it only when xcrun -sdk macosx --find metal succeeds.
    • Because of that, mlx is fetched, and a failed fetch exits the install, even when MLX will not be built:
      • on Macs that have only the Command Line Tools;
      • with EXECUTORCH_BUILD_MLX=OFF;
      • with --minimal, which forces MLX off (setup.py:171).
    • At least correct the comment. Better, skip mlx when EXECUTORCH_BUILD_MLX=OFF is set or xcrun --find metal fails.
  • install_executorch.py:137-138: new keys widen the fragile substring match (minor).
    • "json" and "mlx" are matched with submodule in path. Each one matches exactly one path today.
    • Any future submodule whose path contains those strings would quietly become required, for example another backends/mlx/third-party/*. With the targeted update, that now triggers a real fetch.
    • Matching the last path component (os.path.basename(path) == submodule) is safer.
  • .gitmodules:74: shallow = true on llama.cpp doesn't belong in this change (minor).
    • After this PR, install_executorch.py never fetches llama.cpp. The flag only affects git clone --recurse-submodules and manual git submodule update.
    • It changes how everyone else clones llama.cpp:
      • Bumping the pin starts from a clone with no history.
      • llama.cpp's build-info derives its build number from git history, which a shallow clone may get wrong. This is cosmetic and I did not verify it, because the submodule isn't checked out here.
    • Please split it out, or give the reason in the PR description.

Recommendation

Request Changes

Before merging, keep Vulkan from dropping out with no warning, and fix the documented EXECUTORCH_BUILD_VULKAN / VGF installs on clones without submodules, either by fetching their submodules when they are requested or detected, or by saying clearly what was skipped. The other findings are minor.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@JakeStevens