Detect ATen when a target names it by its resolved label - #22553
Merged
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22553
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 8a5fa7e with merge base de3f49d ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
added
the
release notes: build
A target can name a third-party dependency two ways. It can give the short name, which goes in `external_deps` and is resolved later, or it can call `external_dep_location`, which hands back the resolved label so the target puts it in an ordinary `deps` list. The check for whether a target compiles against ATen only looked at the first, so a target using the second was treated as if it had no ATen dependency and was compiled at C++17. PyTorch's headers need C++20 now, so those targets are left compiling against headers they cannot. The Vulkan operator tests are the ones this reaches. They name libtorch through `external_dep_location`, and the open source Buck build cannot query that backend at all, so nothing here builds them and the mismatch does not surface until someone does. The same tests already ask for C++20 in their CMake build, so the requirement is not in question, only the Buck side of it. Resolve the same names the check already knows and compare against the labels they resolve to. A dependency list can be a select rather than a plain list, so collect through the helper the file already uses for that instead of walking it. Test Plan: Added a unit test for the decision, next to the other tests of build-script logic and for the same reason: the targets it matters for cannot be built here, while the decision itself is a plain function of a target's arguments. It covers a resolved label in `deps` and in `exported_deps`, the same label inside a select, every short name in `external_deps`, a plain target with no ATen dependency, a select holding no ATen dependency, and a target whose only dependency is a project label, since every one of those contains the word torch and an earlier version of this check matched on that substring. Confirmed the test fails without the change. Two of its cases fail against the previous version of the file, and the select case fails against a version that walks the list directly, which is how the select problem was found.
shoumikhin
force-pushed
the
vulkan-buck-cxx20
branch
from
September 4, 2026 07:42
675750e to
c2c5eb6
Compare
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The fix closes the actual representation gap: ATen dependencies can enter through resolved labels in deps/exported_deps, not only short names in external_deps. Resolving the existing ATen names and then walking selects avoids substring heuristics that would misclassify ordinary ExecuTorch labels. The negative //executorch/... case is particularly useful because it guards against exactly that over-broad implementation.
Contributor
Author
|
Thanks for reading it closely. On the negative case, that is exactly why it is there. An earlier version of this check |
JacobSzwejbka
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A target can name a third-party dependency two ways. It can give the short name, which
goes in
external_depsand is resolved later, or it can callexternal_dep_location,which hands back the resolved label so the target puts it in an ordinary
depslist.The check for whether a target compiles against ATen only looked at the first. A target
using the second was treated as having no ATen dependency and was compiled at C++17.
PyTorch's headers need C++20 now, so those targets are left compiling against headers
they cannot.
The Vulkan operator tests are what this reaches. They name
libtorchthroughexternal_dep_location, and the open source Buck build cannot query that backend atall, so nothing here builds them and the mismatch does not surface until someone does.
Those same tests already ask for C++20 in their CMake build, so the requirement is not
in question, only the Buck side of it.
This resolves the same names the check already knows and compares against the labels
they resolve to.
Test plan
Added a unit test for the decision, next to the other tests of build script logic and
for the same reason: the targets it matters for cannot be built here, while the decision
itself is a plain function of a target's arguments.
It covers a resolved label in
depsand inexported_deps, every short name inexternal_deps, a plain target with no ATen dependency, and a target whose onlydependency is a project label, since every one of those contains the word torch and an
earlier version of this check matched on that substring.
The test fails without the change: two of its five cases fail against the previous
version of the file, and all five pass with it.