[zingo]

Use kernel size 5 for padding 2 in the AvgPool and MaxPool tests. The default kernel size 3 makes PyTorch reject these cases before backend execution, causing the shared harness to skip them.

Authored with OpenAI Codex.

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

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 1 New Failure, 1 Pending

As of commit 9a17274 with merge base b3b565b ():

NEW FAILURE - The following job has failed:

• pull / android / run-emulator (gh)
  adb: failed to install android-test-debug-androidTest.apk: cmd: Failure calling service package: Broken pipe (32)

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

[zingo]

Hi @Gasoonjia is this for you to review?

[zingo]

@claude review effort=max

[claude]

Claude finished @zingo's task in 14m 23s —— View job

---

Approve, with one cheap tweak: all four changed calls now pass PyTorch's pad <= kernel_size // 2 check, but stride defaults to the kernel size (5), so the new padding=2 cases never read the trailing pad; adding stride=1 would make them actually test two-sided padding.

Detailed review

PR Review: #25034

Summary

The PR uses kernel_size=5 so the padding=2 subtests in the 1D/2D AvgPool/MaxPool suite tests run in eager PyTorch, which stops the harness from skipping them. Nothing blocks merging. The main issue is that the new cases never reach the trailing padding. The other three items are follow-ups: the harness skip path that hid these cases, a missing test plan, and a pre-existing dead test in the same files.

Testing

1. The new padding=2 cases never read the trailing (right/bottom) pad. Affects test_avgpool1d.py:88, test_maxpool1d.py:90, test_avgpool2d.py:96 and test_maxpool2d.py:95.

   • Model passes stride=None, so stride equals kernel_size, which is now 5.
   • 1D (L=100, k=5, s=5, p=2): the 20 windows are [5i-2, 5i+2] and the last is [93, 97]. Pad positions 100–101 are never read, and neither are inputs 98–99.
   • 2D (20×20, p=(1, 2)): the last windows are rows [14, 18] and columns [13, 17].
   • A lowering that emits pads (2, 0) gives the same shape and values, so it passes.
   • On Arm, adjust_pooling_pad_if_needed (backends/arm/operators/operator_validation_utils.py:229-239) gives post-pad −2 for 1D and for W, and −1 for H. So on Arm the case tests the negative post-pad path, not a pad of 2 on both sides.
   • Fix: add stride=1. The post-pad then equals the pre-pad, and both edges are read. For 2D, kernel_size=(3, 5) is the minimal change, since only W needed k ≥ 5:

   Model(kernel_size=5, stride=1, padding=2)            # 1D
   Model(kernel_size=(3, 5), stride=1, padding=(1, 2))  # 2D

   Fix this →

2. Follow-up: the mechanism that hid these cases is still there.

   • runner.py:107-111 turns any eager-mode exception into TestResult.SKIPPED. Export failures take the same path at runner.py:136-137.
   • TestRunner.lower_and_run_model (conftest.py:103-110) then calls pytest.skip. That marks the whole test_*_padding node as skipped on every flow, including the valid padding=1 subtest that already ran, and it stops any later subtests.
   • This PR fixes four instances by hand. The next invalid case will be just as invisible.
   • Suggestion: add a separate TestResult for eager-reference failures that fails the test. Cases that legitimately can't run on a flow can go in that flow's skip_patterns, as flows/coreai.py:45-48 already does.
   • This does not block this PR.

   Fix this →

3. There is no test plan, and the re-enabled subtests have only run on the flows PR CI uses.

   • CONTRIBUTING.md:403 asks for repeatable test instructions.
   • PR CI runs a reduced set of flows: arm_tosa_fp and arm_vgf_fp (test-backend-arm.yml:28-31,47-49), plus xnnpack, vulkan and qnn. Those passed.
   • These four subtests have never run on any flow before. The nightly-only flows will first see them after merge: arm_tosa_int, arm_ethos_u55/arm_ethos_u85, arm_vgf_int, and the quantized XNNPACK/QNN/Vulkan flows.
   • Suggestion: before merging, add the ciflow/nightly label (.github/pytorch-probot.yml:12), or list the flows you ran locally.

4. Pre-existing, in the same files but outside the diff: the return_indices tests don't test return_indices.

   • test_maxpool1d.py:116, test_maxpool2d.py:121 and test_maxpool3d.py:115 each define ModelWithIndices(return_indices=True) and never use it.
   • They run Model(kernel_size=3, stride=2, padding=1) instead, which has return_indices=False.
   • It's the same kind of bug this PR fixes: a test that quietly doesn't cover what its name says. Fine as a follow-up.

   Fix this →

Recommendation

Approve. The change is correct and only adds coverage. Item 1 is a small edit worth making before merge, so the cases test what their names say. Items 2–4 can be follow-ups.

Review steps

• Loaded the /pr-review skill and gathered context
• Three independent reviewers each checked every changed line against the checklist
• Merged duplicate findings (items 1, 2 and 4 were raised by all three reviewers, item 3 by two)
• Fact-checked against the changed files, the harness (runner.py, conftest.py, reporting.py), the Arm pad helper, and the CI flow config
• Posted the review

I couldn't run Python here, so the window and pad numbers are worked out by hand from PyTorch's output-size formula and the Arm helper's source, not executed. The PyTorch pad <= kernel_size // 2 rule comes from ATen's pooling shape checks. I didn't reproduce it.