Skip to content

Add Arm Buck2 coverage and declaration checks to GitHub CI - #23888

Open
usamahz wants to merge 2 commits into
pytorch:mainfrom
usamahz:codex/arm-buck2-ci
Open

usamahz wants to merge 2 commits into
pytorch:mainfrom
usamahz:codex/arm-buck2-ci

Conversation

@usamahz

@usamahz usamahz commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The public Buck2 suite can stay green when Vela tests depend on initialization by an earlier test or when Arm Python imports are missing Buck declarations. Run the Vela parser suite through the Buck2 job and run each discovered case in a fresh process. Initialize the runtime in the fixture and enable a plain test main only on the isolated OSS Buck target. Preserve the existing internal target label for individual-case selection and leave internal and CMake test entrypoints intact.

Add a separate GitHub job and a shared local test_buck_declarations entrypoint to catch newly introduced dependency and test-registration gaps. The checker follows supported in-repository declarations and transitive dependencies and reports declaration errors after lazy package loading, with their original file and unsupported construct. Confirmed missing dependencies, unregistered Arm tests, and unsupported Arm declarations fail; unsupported declarations outside Arm and dependency gaps that cannot be proven because of them produce warnings. Baseline comparison includes severity so warnings becoming errors still fail. The README documents this policy and maintenance by Arm backend contributors.

This supplements Meta's internal packaged Python tests; it does not reproduce their environment. This change is independent of #22580.

Authored with assistance from OpenAI Codex.

Test plan

  • python3 -m unittest discover -s .ci/scripts/tests -p 'test_arm_buck*.py' — all 65 tests pass: 53 checker tests and 12 shell entrypoint tests.
  • BASE_REVISION=2a84702c2ce453d095e784a9af9403bc87987dd4 bash backends/arm/test/test_arm_backend.sh test_buck_declarations — passes with zero new errors or warnings and 282 unchanged findings.
  • Pinned UFMT, Flake8, ClangFormat, and git diff --check pass for the edited code.
  • Focused Apple Clang compile/link checks using repository-pinned GoogleTest pass with and without the OSS define: the enabled object supplies a main and links with ExecuTorch's weak public main; the disabled object has no main and links with Google's standard main. All seven Vela cases pass together and individually in both binaries.
  • A temporary negative-control build without fixture initialization reproduces the expected PAL initialization failure, confirming that the OSS isolation check still detects it.

These C++ checks use public sources and do not verify Meta's internal build. A complete Buck2 build was not rerun locally for this revision; the GitHub Buck2 job remains the full build check.

cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell @rascani

@pytorch-bot

pytorch-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ You can merge normally! (4 Unrelated Failures), 2 Unclassified Failures

As of commit aa6b089 with merge base 7da27ca (image):

UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:

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

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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 Oct 15, 2026
@github-actions github-actions Bot added ciflow/trunk module: arm Issues related to arm backend labels Oct 15, 2026
@usamahz usamahz added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm release notes: arm Changes to the ARM backend delegate labels Oct 15, 2026
@usamahz
usamahz requested a review from zingo October 15, 2026 09:47
@usamahz usamahz self-assigned this Oct 15, 2026
@usamahz usamahz added the help wanted Extra attention is needed label Oct 15, 2026
@usamahz
usamahz force-pushed the codex/arm-buck2-ci branch from 609ca3a to 7edb8d7 Compare October 15, 2026 10:20
@zingo

zingo commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

hi @rascani and @digantdesai we hope something like this could catch some buck2 stuff on github when merged :)

@zingo

zingo commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

New job runned pull / test-arm-backend-buck-declarations (pull_request)Successful in 53s

/home/runner/work/executorch/executorch
Run Arm backend test suite test_buck_declarations
EXECUTORCH_VGF_VULKAN_VALIDATION=UNSET
VK_INSTANCE_LAYERS=UNSET
VK_LAYER_PATH=UNSET
VK_ADD_LAYER_PATH=UNSET
VULKAN_SDK=UNSET
...........................................
----------------------------------------------------------------------
Ran 43 tests in 0.076s

OK
Arm Python Buck declaration check: PASS
  New issues: 0
  Existing issues unchanged: 257

@rascani

rascani commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

@claude review this PR

@claude

claude Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

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


The PR is functional, but one bug in analyze() and one design question about failing unrelated PRs need resolving before merge; the remaining two points are minor.

Detailed review

PR Review: #23888

Summary

The PR runs each Vela parser case in its own process under Buck2, moves that C++ test into its own vela package, and adds a static checker for Arm Python Buck declarations. The checker runs in a new ubuntu-latest job and is compared against a base-revision baseline. The checker has a bug that drops the real cause of out-of-scope failures. The fail policy can also block PRs from other teams over valid Buck code that the checker doesn't model.

Code Quality

  • Out-of-scope package errors are collected too early and never reported. In .ci/scripts/check_arm_buck_declarations.py:666-670, analyze() builds the cannot validate issues from declarations.package_errors right after load_scope(). Packages outside the scope, like exir/ or devtools/, are only loaded later and lazily, through reachable()/get() (:475-478) and owners() (:480-495). Any unsupported declaration loop, parameterized declaration helper or conditional declaration error from those packages goes into the dict after it has already been read, so it is lost.
    • What the user sees instead is a follow-on no supported python_library provider for executorch.exir... or unresolved dependency declarations message that doesn't name the file or construct at fault.
    • Fix: collect package_errors at the end of analyze(). Optionally limit it to packages that host a provider or a dependency that Arm needs.
  • Two cxx_test targets now build the same source, with deps copied between them. backends/arm/test/targets.bzl:199-208 defines the internal target with a cross-package srcs reference. backends/arm/test/vela/targets.bzl:14-24 defines an OSS-only copy with the same four deps, which must now be kept in sync by hand. A single unconditional cxx_test in backends/arm/test/vela/ would remove the export_file and the visibility allow-list. Is the internal label //executorch/backends/arm/test:vela_external_blocks_test referenced by something that stops it from moving? If so, a comment explaining that would help.

Infrastructure

  • Needs discussion: the new job can fail PRs that never touch Arm. The checker follows transitive deps into any in-repo package. When it meets valid Buck it doesn't model, it raises a fatal cannot validate (:436-463, :425-429, :220). Examples are a for loop of python_library, a helper called with arguments, or a computed srcs.
    • So a correct refactor of exir/BUCK, devtools/... or backends/test/harness/... turns into "new issues" for Arm libraries that import those modules, and test-arm-backend-buck-declarations fails. The only fix available to that author is to change an 870-line Arm-specific Starlark interpreter they don't own.
    • Option 1: fail only on missing dependency, unregistered test, and cannot validate for declarations under backends/arm. Out-of-scope cannot validate would become a warning.
    • Option 2: scope the job to PRs that touch backends/arm/**.
    • Either way, it would help to say who maintains the interpreter as Buck usage changes. The longer-term fix is still the TODO at .ci/scripts/unittest-buck2.sh:13 ("Make //backends/arm tests use runtime wrapper so we can just query"), which would let buck2 uquery do this check.
  • Low confidence: the new strong main() also replaces the internal test main. vela_external_blocks_test.cpp:322-328 adds a strong main(). The comment frames it as overriding "the default OSS Buck test main" (the weak main in test/utils/UnitTestMain.cpp). But the same source is now compiled into the internal //executorch/backends/arm/test:vela_external_blocks_test, where it will also replace whatever main fbcode's cxx_test links. Please confirm that the internal build links cleanly and doesn't skip any init that main performs. The runtime_init() fixture in SetUp() is enough on its own to make each case independent. Dropping the custom main() would also work, at the cost of the per-case loop in unittest-arm-buck2.sh no longer catching a missing fixture.

Recommendation

Needs Discussion

The early package_errors collection is a straightforward fix. Before this becomes a required check on every PR, maintainers should agree on the fail policy for out-of-scope cannot validate findings.

@usamahz
usamahz force-pushed the codex/arm-buck2-ci branch from 68b1d1a to 494b81a Compare October 19, 2026 10:22
@usamahz

usamahz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the Claude's review report!


def visit(statements: list[ast.stmt], env: dict[str, object]) -> None:
for node in statements:
if isinstance(node, ast.FunctionDef) and node.name == "define_arm_tests":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we count registrations only when define_arm_tests is actually invoked? Traversing the function definition unconditionally lets the suite disappear without this check noticing.


def visit(statements: list[ast.stmt], local_env: dict[str, object]) -> None:
for node in statements:
if (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This visitor silently ignores augmented assignments such as srcs += ["new_helper.py"], retaining the old source list. Could we evaluate these mutations or mark the affected value unsupported so the checker reports that it cannot validate the declaration?

package[: len(package) - node.level + 1]
+ ([module] if module else [])
)
if not module.startswith("executorch."):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this guard also accept module == "executorch"? Currently, from executorch import exir returns before resolving the imported alias, so a missing dependency passes unnoticed. The equivalent import executorch.exir correctly reports the missing dependency. Allowing the root package through should let the existing child-module resolution handle this case.

usamahz added a commit to usamahz/executorch that referenced this pull request Nov 3, 2026
Count Arm test registrations only when the declaration visitor actually
invokes define_arm_tests. Keep the package, source file and original
helper name so aliases and called wrappers work while unused, inactive
or unsupported calls cannot keep a removed test suite falsely registered.

Evaluate supported augmented assignments to source and dependency lists.
Invalidate unsupported mutations and report the original expression
instead of retaining stale declaration values. Resolve imports from the
executorch root package through the same child-module dependency checks
as dotted imports.

Add regression coverage for all three new review comments on GitHub
PR pytorch#23888. The 64 checker tests and 12 entrypoint integration tests pass.
The local entrypoint reports zero new errors or warnings against the
parent patch. Removing the real Arm test macro call in a temporary
snapshot reports 154 newly unregistered tests. UFMT, Flake8 and diff
whitespace checks pass.

Authored with assistance from OpenAI Codex.

Signed-off-by: Usamah Zaheer <usamah.zaheer@arm.com>
Change-Id: I6944d6481c6231754269716e4db1e3d7dcf22272
The public Buck2 suite omits Arm test coverage, so it can stay green
when isolated Vela tests fail or Arm Python imports lack Buck
declarations. Run the existing Vela parser suite and each discovered
case in a fresh process. Retain runtime initialization in the test fixture
before every case. Enable a plain test main only on the OSS isolation
target so global initialization cannot mask missing fixture setup,
while preserving internal and CMake test entrypoints.

Move the C++ test into a standalone Buck package to avoid the internal
Python test helper. Preserve the existing internal target label for
individual-case selection, declare the platform dependency and update
CMake's source path. Cover all ten Vela tests on upstream/main, including
the malformed block and I/O payload cases.

Add test-arm-backend-buck-declarations beside the other Arm GitHub jobs.
Run it through test_arm_backend.sh test_buck_declarations so developers
and CI share a command requiring only Git and Python. Compare against
the PR merge base, previous push revision or local HEAD^, with
BASE_REVISION available for changes spanning several commits. Keep the
declaration check independent of Arm tool setup and the all suite.

Check Arm Python test-library dependencies and test registration using
supported local Buck declarations and their transitive dependencies.
Discover every BUCK/TARGETS package beneath the Arm test tree so
standalone nested libraries and removal of their source ownership are
checked too. Collect errors after lazy dependency and provider discovery
to report their actual build or helper file and unsupported construct.

Preserve existing findings as a baseline. Fail on confirmed missing
dependencies, unregistered Arm tests and unsupported Arm declarations.
Report unsupported declarations outside Arm and dependency gaps that
cannot be proven because of them as warnings. Include severity in the
baseline identity so a warning becoming an error still fails. Document
maintenance by Arm backend contributors and the limited static model.
Evaluate Boolean expressions lazily, follow helpers only in active
internal branches and reject parameterized or recursive helpers that
cannot be validated. Present numbered findings with severity, files,
targets and wrapped details. This check supplements Meta's internal
packaged Python tests.

Validation: 53 checker tests and 12 shell entrypoint integration tests
pass, covering late diagnostics, warning policy, severity escalation,
baseline selection, setup isolation and failure propagation. The local
entrypoint reports zero new errors or warnings against upstream/main. UFMT, Flake8, ClangFormat and git diff --check pass. Focused
public C++ compile/link checks cover both main configurations and all
ten Vela cases together and separately. Meta's internal build and a
complete Buck2 build have not been run for this amendment.

Authored with assistance from OpenAI Codex.

Signed-off-by: Usamah Zaheer <usamah.zaheer@arm.com>
Change-Id: I2cdf857306becd0a1eeb844763b4a96a88d33502
Count Arm test registrations only when the declaration visitor actually
invokes define_arm_tests. Keep the package, source file and original
helper name so aliases and called wrappers work while unused, inactive
or unsupported calls cannot keep a removed test suite falsely registered.

Evaluate supported augmented assignments to source and dependency lists.
Invalidate unsupported mutations and report the original expression
instead of retaining stale declaration values. Resolve imports from the
executorch root package through the same child-module dependency checks
as dotted imports.

Add regression coverage for all three new review comments on GitHub
PR pytorch#23888. The 64 checker tests and 12 entrypoint integration tests pass.
The local entrypoint reports zero new errors or warnings against the
parent patch. Regression tests cover removed and inactive registration
calls, augmented assignments and imports from the root package. UFMT,
Flake8 and diff whitespace checks pass.

Authored with assistance from OpenAI Codex.

Signed-off-by: Usamah Zaheer <usamah.zaheer@arm.com>
Change-Id: I6944d6481c6231754269716e4db1e3d7dcf22272
@usamahz
usamahz force-pushed the codex/arm-buck2-ci branch from e82cda3 to aa6b089 Compare November 3, 2026 17:28

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

Labels

ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. help wanted Extra attention is needed module: arm Issues related to arm backend partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm release notes: arm Changes to the ARM backend delegate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants