migrate backends/apple/coreml (TARGETS+BUCK both with rules) (#21429) - #21429
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21429
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 143 PendingAs of commit 11e356a with merge base d8c6be2 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@bigfootjon has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109082057. |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR continues the migration away from standalone TARGETS files by consolidating fbcode-only and xplat-only Buck targets into targets.bzl (with define_common_targets(is_fbcode=...)) and switching directories to a thin BUCK caller that gates behavior via is_fbcode().
Changes:
- Introduce/extend
define_common_targets(is_fbcode = False)patterns in several directories to preserve fbcode-vs-xplat behavior while deletingTARGETS. - Add thin
BUCKentrypoints that calldefine_common_targets(is_fbcode = is_fbcode()). - Migrate
backends/apple/coremlto a singletargets.bzlwith explicit fbcode/xplat branching and simplify itsBUCK.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/pytree/test/targets.bzl | New gated target definitions (fbcode-only) moved out of TARGETS. |
| extension/pytree/test/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| extension/pytree/test/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| examples/models/llama/config/targets.bzl | Gate existing unittest target definitions on is_fbcode. |
| examples/models/llama/config/TARGETS | Deleted; migration to BUCK+targets.bzl calling convention. |
| examples/models/llama/config/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| backends/cuda/tests/targets.bzl | New gated CUDA test target definitions moved out of TARGETS. |
| backends/cuda/tests/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| backends/cuda/tests/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| backends/cuda/runtime/targets.bzl | New gated CUDA runtime + tests target definitions moved out of TARGETS. |
| backends/cuda/runtime/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| backends/cuda/runtime/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| backends/cuda/runtime/shims/tests/targets.bzl | Update common-targets entrypoint to accept is_fbcode gating. |
| backends/cuda/runtime/shims/tests/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| backends/cuda/runtime/shims/tests/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| backends/apple/coreml/targets.bzl | New combined fbcode/xplat target definitions with explicit branch split. |
| backends/apple/coreml/TARGETS | Deleted after moving fbcode Python rules into targets.bzl. |
| backends/apple/coreml/BUCK | Reduced to a thin caller into targets.bzl gated by is_fbcode(). |
| backends/aoti/tests/targets.bzl | New gated AOTI test target definitions moved out of TARGETS. |
| backends/aoti/tests/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| backends/aoti/tests/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
| backends/aoti/slim/factory/test/targets.bzl | Gate existing slim factory tests on is_fbcode to preserve prior behavior. |
| backends/aoti/slim/factory/test/TARGETS | Deleted in favor of targets.bzl + thin BUCK. |
| backends/aoti/slim/factory/test/BUCK | New thin caller invoking define_common_targets(is_fbcode=...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ":cuda_platform", | ||
| ":runtime_shims", | ||
| ":cuda_allocator", | ||
| ":cuda_platform", | ||
| "//executorch/backends/aoti:aoti_common_slim", |
| "//executorch/extension/tensor:tensor", | ||
| ], |
| load("@fbsource//xplat/executorch/build:runtime_wrapper.bzl", "runtime") | ||
| load("@fbcode_macros//build_defs:python_unittest.bzl", "python_unittest") | ||
|
|
| load("@fbsource//xplat/executorch/build:runtime_wrapper.bzl", "runtime") | ||
| load("@fbcode_macros//build_defs:python_unittest.bzl", "python_unittest") | ||
| load("@fbcode_macros//build_defs:python_unittest_remote_gpu.bzl", "python_unittest_remote_gpu") | ||
| load("@fbcode_macros//build_defs/lib:re_test_utils.bzl", "re_test_utils") | ||
|
|
ndmitchell
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
a0ef20e to
30c7481
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
backends/apple/coreml/targets.bzl:15
- This comment is now redundant/misleading inside targets.bzl (it says targets must be defined in targets.bzl). Since this file itself is targets.bzl and the branch is specifically fbcode-only, it would be clearer to replace it with a short branch-specific note.
# Any targets that should be shared between fbcode and xplat must be defined in
# targets.bzl. This file can contain fbcode-only targets.
backends/apple/coreml/BUCK:2
- The top-of-file comment still describes targets.bzl as holding only shared targets, but the current setup delegates all targets (including fbcode-only/xplat-only branches) to targets.bzl. Updating the comment would avoid confusion for future maintainers.
# Any targets that should be shared between fbcode and xplat must be defined in
# targets.bzl.
30c7481 to
c3b54a4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
runtime/core/BUCK:4
- This PR changes runtime/core/BUCK to load its target definitions from
@fbsource//xplat/executorch/runtime/core:targets.bzl(shim_et in OSS), which is a broader migration than the PR description (focused on CoreML) indicates. Consider adding a brief note here clarifying why the targets live in the xplat shim to avoid confusion for future maintainers.
# Any targets that should be shared between fbcode and xplat must be defined in
# targets.bzl. This file can contain xplat-only targets.
load("@fbsource//xplat/executorch/runtime/core:targets.bzl", "define_common_targets")
This enables #21429 to work correctly (see failing BUCK CI job there) It's currently failing due to `targets.bzl` being exported from Meta to the wrong location. This fixes that on GH.
c3b54a4 to
c67db23
Compare
…#21429) Summary: TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
…#21429) Summary: Pull Request resolved: pytorch#21429 TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
c67db23 to
11e356a
Compare
…#21429) Summary: TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
…#21429) Summary: TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
…#21429) Summary: TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
…#21429) Summary: TARGETS contained fbcode-only Python rules (placeholder backend imports); BUCK contained xplat-only Apple platform cxx_library targets. Created a new targets.bzl with define_common_targets(is_fbcode = False) whose body splits via if/else: fbcode branch holds the Python rules, xplat branch holds the Apple cxx targets. Replaced BUCK with thin caller, deleted TARGETS. Reviewed By: ndmitchell Differential Revision: D109082057
Summary:
TARGETS contained fbcode-only Python rules (placeholder backend imports);
BUCK contained xplat-only Apple platform cxx_library targets. Created a
new targets.bzl with define_common_targets(is_fbcode = False) whose body
splits via if/else: fbcode branch holds the Python rules, xplat branch
holds the Apple cxx targets. Replaced BUCK with thin caller, deleted
TARGETS.
Reviewed By: ndmitchell
Differential Revision: D109082057