Skip to content

Fix parallel_for_no_threadpool invoking the callback for an empty range - #22869

Open
psiddh wants to merge 1 commit into
pytorch:mainfrom
psiddh:fix-parallel-for-empty-range
Open

psiddh wants to merge 1 commit into
pytorch:mainfrom
psiddh:fix-parallel-for-empty-range

Conversation

@psiddh

@psiddh psiddh commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Fixes the unittest-release failure that has been red on main since Sep 14, on linux, macOS and windows.

The bug

#22663 added NestedCallsPreserveOuterThreadNumber, which asserts that an empty nested range
never invokes its callback, and parameterized it over both parallel_for implementations. It
added the guard to one of them:

  • extension/threadpool/thread_parallel.cpp — got if (begin < end) { f(begin, end); }
  • runtime/kernel/thread_parallel_interface.h — untouched by that PR ❌

In the second, parallel_for_no_threadpool does:

#ifndef NDEBUG
  for (const auto i : c10::irange(begin, end)) { ... f(idx, idx + 1); }  // empty range -> no call
#else
  f(begin, end);                                                         // <- unconditional
#endif

Debug builds pass only by accident — c10::irange(2, 2) is empty, so the callback is skipped as a
side effect. Release builds call it, and the test fails ten times with
Empty nested range invoked its callback.

That is exactly the observed CI signature: unittest green on all three platforms, unittest-release
red on all three, on every affected commit. Platform-independent, as a pure #ifdef divergence
should be.

The fix

Check begin == end above the #ifdef rather than duplicating the guard into the release arm, so
the two branches agree by construction instead of by coincidence.

Why it matters beyond the test

unittest-release lives in the trunk workflow, and trunk is in update-viablestrict's
requires list. One red job reddens the workflow, which disqualifies the commit, so
viable/strict has not advanced since Sep 14 (51 commits). This is one of two blockers; the other
is the Samsung job, fixed separately in #22870.

Verification

Reproduced against the real header — DEBUG: 0 callback invocations, RELEASE: 1. Then built the
actual extension/threadpool/test/thread_parallel_test.cpp:

before after
release NestedCallsPreserveOuterThreadNumber FAILS 26/26 pass
debug 26/26 pass 26/26 pass

No new test is needed — #22663's test already covers this and is the one that was failing.

Caveat: my local harness linked without ET_USE_THREADPOOL, so the /0 variant routed through the
same function as /1. The /1 case, which is the actual CI failure, is faithful. I did not run it
on Linux or Windows.

cc @JakeStevens @digantdesai — author and reviewer of #22663.


Authored with Claude Code.

pytorch#22663 added NestedCallsPreserveOuterThreadNumber, which asserts that an
empty nested range never invokes its callback, and parameterized it over
both parallel_for implementations. It added the guard to the threadpool
one in extension/threadpool/thread_parallel.cpp but not to this one, so
the release build calls f(begin, end) unconditionally and the test fails
with "Empty nested range invoked its callback".

Debug builds pass only by accident: they iterate c10::irange(begin, end),
which is empty, so the callback is skipped as a side effect. Checking
begin == end up front makes both branches agree by construction rather
than by coincidence, which is why the check is above the #ifdef rather
than duplicated into the release arm.

This is what has been holding viable/strict since Sep 14: unittest-release
fails on linux, macOS and windows, which reddens the trunk workflow, and
trunk is required by update-viablestrict.
Copilot AI lite review requested due to automatic review settings September 16, 2026 08:19
@pytorch-bot

pytorch-bot Bot commented Sep 16, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 Pending, 1 Unclassified Failure

As of commit 0d06d2c with merge base 63d5e55 (image):

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

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 Sep 16, 2026
@psiddh psiddh added the release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs label Sep 16, 2026

Copilot AI left a comment

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.

🟢 Approval recommended

No unresolved review comments remain.

Pull request overview

Fixes release-only callback invocation for empty ranges in parallel_for_no_threadpool.

Changes:

  • Adds a shared guard for begin == end.
  • Aligns debug and release behavior.
File summaries
File Description
runtime/kernel/thread_parallel_interface.h Prevents callbacks for empty ranges in all build modes.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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. release notes: runtime Changes related to the core runtime which loads the program methods, initializes delegates, and runs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants