Skip to content

Reject an unsupported ReduceOp before any message; remove the opt-in COMMUX_SPIN_US spin (#13, #12) - #14

Merged
zoeyzyhu merged 7 commits into
zoeyzyhu:mainfrom
UCzhangxi:xiz/reduce-op-check
Sep 30, 2026
Merged

zoeyzyhu merged 7 commits into
zoeyzyhu:mainfrom
UCzhangxi:xiz/reduce-op-check

Conversation

@UCzhangxi

@UCzhangxi UCzhangxi commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #13
Closes #12

Seven commits on main 0994e4e: three fix #13, one removes the opt-in COMMUX_SPIN_US spin (which resolves #12), and three harden CI.

#13: an unsupported ReduceOp must fail on every rank before any message

apply_reduce rejected an op other than SUM, PRODUCT, MIN or MAX only on a rank that had already received data. In the binomial tree the leaves sent and returned, so some ranks raised, some returned and some hung, and the unmatched messages reached the next collective with the same tag.

  • 84abe19 (test): tests/test_reduce_op.py calls reduce at every root and all_reduce with AVG and BAND. Every rank must raise, and a SUM afterwards must still be right. CI runs it at 1, 3 and 4 ranks under timeout 120.
  • b07e3d2 (fix): check_reduce_op() at the top of allreduce() and reduce(), before the lock and before any message. This also runs at world size 1, so an unsupported op now raises there too, where it used to return the tensor unchanged. No call in snapy main (3f7ad96) or in our downstream runner uses an op outside SUM, MIN or MAX.
  • 10020f7 (tests): test_collectives.py also covers PRODUCT, MIN and MAX at every root, a 2 MiB all_reduce (rendezvous path) and a three-tensor all_reduce. CI runs it at 3 to 7 ranks.

test_reduce_op.py on the test commit alone (main's source) vs with the fix:

ranks   before                                   after
  3     rc 1, 11 FAIL lines                      rc 0, 0 failures
  4     rc 124 (hung until the 120 s timeout)    rc 0, 0 failures
  5     rc 1, 14 FAIL lines                      rc 0, 0 failures

#12: remove the opt-in spin

99a6347 removes COMMUX_SPIN_US: spin_us(), SpinBudget, the deadline passed to worker_wait_timed, its sleep on UCS_ERR_BUSY, and the spin branches in UCXWork::wait and wait_request, together with the spin-only checks in tests/test_commux.cpp. The spin was off by default, no configuration we know of sets it, and each review of the path found another case to handle. #12's extra poll only happened with the spin on, so removing the spin resolves #12 and returns the wait path to what it was before #11.

Check: git diff cf41b46 HEAD -- src contains only check_reduce_op (with its two calls) and the binomial-tree reduce_locked and broadcast_locked from #11. worker_wait_timed, UCXWork::wait and wait_request are byte-identical to cf41b46. The build fixes from #11 (Threads for test_commux, torch's pybind11 ABI tag) stay.

The spin-path fixes that Sihe Chen worked out with the reviewers (reap before parking, parking a collective under a flood, a TCP-only park, multi-request and FIFO tests, relative flood bounds) are kept as a record at UCzhangxi/commux 33d6480, in case the spin is wanted again.

CI

d8fa479 sets UCX_TLS: tcp,sm,self on the two test steps. A hosted runner is one node, so these are the transports the tests use there. One runner (run 36681745872) exposed an RDMA verbs device without SRQ support, and ucp_worker_create failed before any test ran.

eb85378 makes the C++ test step wait on rank 1's process id. A bare wait returns 0, so a failing background rank 1 used to pass the step. It also names the Python step's rank range.

447cddc runs test_reduce_op.py at one rank too, where no message is sent. On main's source that run reports 4 failures (the op returns without an error); with the fix it passes.

Checks at d8fa479 (eb85378 and 447cddc change only CI and a test docstring)

On a Linux CPU node, with the default transports and with UCX_TLS=tcp,sm,self:

  • test_torch_dist.py at 2 ranks, test_reduce_op.py at 3, 4 and 5, test_collectives.py at 3 to 7: all pass, 0 failures.
  • test_commux at 2 ranks with COMMUX_MT=1 COMMUX_DEADLOCK=1: passes, three runs each.
  • clang-format 20.1.4 and every pre-commit hook pass.

With a CUDA build of this head:

  • On a GPU node, test_collectives.py at 6 ranks on CUDA and CPU tensors, and test_torch_dist.py at 2 ranks on CUDA: all pass, 0 failures.
  • A downstream 2-rank hydro run, one on a CPU node and one on a GPU node, with this head preloaded in place of cf41b46 writes the same output: 378 of 378 variables bitwise equal.

Not covered. More than one node.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The low-level UCX concurrency changes affect shared wait paths, while CUDA behavior remains unverified.

Review effort: Balanced
Findings: None

What changed in this PR

Prevents unsupported reductions from communicating and removes an unnecessary polling delay after request completion.

Changes:

  • Validates reduction operations before collective communication.
  • Rechecks completion before parking and improves busy-worker handling.
  • Adds collective, unsupported-operation, latency, and CI coverage.
File Description
src/​process_group_ucx.cpp Implements validation and wait-loop fixes.
tests/​test_reduce_op.py Tests unsupported reductions across ranks.
tests/​test_commux.cpp Adds late-poll latency regressions.
tests/​test_collectives.py Expands operation, size, and payload coverage.
.github/​workflows/​ci.yml Runs expanded CPU regression tests.

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

The spin was opt-in and no configuration sets it. It added a per-wait
budget, a deadline through worker_wait_timed and extra branches in both
wait loops, and each review of that path found another case to handle.
worker_wait_timed, UCXWork::wait and wait_request are back to the
default path they always took with COMMUX_SPIN_US unset. The spin-budget
checks in test_commux go with it. The binomial-tree collectives and the
build fixes stay.
A hosted runner is one node, so these are the transports the tests use
there. A runner that exposes a verbs device without SRQ support fails
ucp_worker_create before any test runs (run 36681745872, centralus).
@UCzhangxi UCzhangxi changed the title Reject an unsupported ReduceOp before any message; no extra poll after the spin budget (#12, #13) Reject an unsupported ReduceOp before any message; remove the opt-in COMMUX_SPIN_US spin (#13, #12) Sep 30, 2026
A bare wait returns 0, so a failing background rank 1 passed the step. Also name
the Python step's actual rank range (2-7).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

CI does not cover the newly guaranteed single-rank rejection behavior.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread .github/workflows/ci.yml Outdated
check_reduce_op runs before the size_ > 1 guard, so an unsupported op must be
rejected at world size 1 as well, where no message is sent.
@zoeyzyhu
zoeyzyhu merged commit 7a192c7 into zoeyzyhu:main Sep 30, 2026
2 checks passed
@UCzhangxi
UCzhangxi deleted the xiz/reduce-op-check branch September 30, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants