Skip to content

[bugfix] Use NPU current streams in PyHcclCommunicator - #2053

Open
Gxj230958 wants to merge 3 commits into
hao-ai-lab:mainfrom
Gxj230958:fix/pyhccl-npu-current-stream-20261102
Open

Gxj230958 wants to merge 3 commits into
hao-ai-lab:mainfrom
Gxj230958:fix/pyhccl-npu-current-stream-20261102

Conversation

@Gxj230958

@Gxj230958 Gxj230958 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

fastvideo.utils.current_stream() returns None on non-CUDA platforms, so PyHcclCommunicator cannot obtain the NPU stream needed for warmup or default collective calls. Use torch.npu.current_stream(self.device) for communicator warmup, all_reduce, and broadcast, while preserving explicitly supplied streams.

The CPU regression tests live in fastvideo/tests/distributed/test_pyhccl_stream.py and are explicitly collected by .buildkite/scripts/unit_test.sh. Five cases cover communicator-device selection, explicit streams, and initialization warmup synchronization. NPU and HCCL calls are mocked in these tests, so they run in the existing GPU CI environment without Ascend hardware.

Validation

  • Buildkite Fastcheck #2277: all six lanes and fastcheck-passed passed at f68a261. Remote documentation and pre-commit checks also passed.
  • Stream regression tests: 5 passed.
  • CI collection and merge-plan contract tests: 59 passed.
  • Real two-rank Ascend 910B2C verification on devices 8 and 9: communicator initialization and warmup, all-reduce and broadcast on a non-default current stream, and both operations with an explicit stream all passed with exact expected values. Runtime: PyTorch 2.9.0 and torch-npu 2.9.0.post1.
  • Two-rank Wan2.1-1.3B training completed communicator initialization and three training updates.
  • Applicable repository pre-commit hooks passed, including mypy.
  • Documentation type checking and formatting passed: npm run check --prefix docs-site and npm run format:check --prefix docs-site.
python -m pytest --confcutdir=fastvideo/tests/distributed \
  fastvideo/tests/distributed/test_pyhccl_stream.py -q
python -m pytest --confcutdir=fastvideo/tests/contract \
  fastvideo/tests/contract/test_ci_test_collection.py \
  fastvideo/tests/contract/test_merge_ci_plan.py -q

CI configuration approval

Maintainer sign-off is required for the change to .buildkite/scripts/unit_test.sh. The change adds fastvideo/tests/distributed/test_pyhccl_stream.py to the existing unit lane, alongside the existing test_sp_warmup_device.py entry.

Before merge, the maintainer needs to explicitly approve this CI configuration change, provide an approving review, and trigger the required merge gate for full-suite-passed.

@mergify

mergify Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI

Protection Waiting on
🔴 PR merge requirements 👀 reviews and 🤖 CI

🔴 PR merge requirements

Waiting for

  • #approved-reviews-by>=1
  • check-success=full-suite-passed
This rule is failing.
  • #approved-reviews-by>=1
  • check-success=full-suite-passed
  • check-success=fastcheck-passed
  • check-success~=pre-commit
  • title~=(?i)^\[(feat|feature|bugfix|fix|refactor|perf|ci|doc|docs|misc|chore|kernel|new.?model|skill|skills|infra)\]

@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

PyHcclCommunicator now gets the current NPU stream for its bound device.

  • PyHcclCommunicator uses its device's current NPU stream for HCCL calls.

No actionable issues found. Tests were not run during this review.

Acknowledged by Gxj230958: remote CI reports an unrelated formatting failure in docs-site/islands/Cookbook.tsx and a microscope test failure whose cause is unknown because the logs require organization access.

Reviews (1) · Last reviewed commit: "[bugfix] Use NPU current streams in PyHc..." · Reviewed by Greptile

@Gxj230958
Gxj230958 marked this pull request as ready for review November 8, 2026 06:21
@fv-superagent

Copy link
Copy Markdown
Collaborator

fv-superagent score: 4/5

The unit lane is red only on a known flake, not on this change.

Build #2268 ends 1 failed, 3590 passed; the failure is test_cudnn_sdpa_dense.py::test_cudnn_route_is_bit_identical_to_cudnn_dispatched_sdpa, a bf16 bit-identity comparison, and that same test passed in build #2265 the same day. This PR touches only pyhccl.py and a new local test, so it cannot affect cuDNN SDPA. /test unit reruns the lane.

On the change: replacing the CUDA-only fastvideo.utils.current_stream() with torch.npu.current_stream(self.device) in PyHcclCommunicator, including the warmup path, is right, and binding the stream to self.device is what the NPU needs.

One note: tests/local_tests/test_pyhccl_stream.py is not collected by any CI lane (tests/local_tests/ is local-only per tests/README.md), so the two regression cases run only by hand. The two-rank Ascend run is the evidence that matters and cannot be reproduced here.

The build check shows a 2026-11-02 failure in the docs-site type step; this PR does not touch docs-site/, so treat that as stale.

— fv-superagent · cudnn flakes, pyhccl gets the blame · run by aryan5v

@mergify mergify Bot added the scope: infra CI, tests, Docker, build label Nov 9, 2026
@fv-superagent fv-superagent mentioned this pull request Nov 9, 2026

@fv-superagent fv-superagent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fv-superagent score: 4/5 (was 4/5)

Re-reviewed head f68a261. You moved the regression test to fastvideo/tests/distributed/test_pyhccl_stream.py and added just that file to .buildkite/scripts/unit_test.sh — the same pattern as the existing test_sp_warmup_device.py entry — and the unit lane passed in build #2277 (fastcheck-passed green). The flake I flagged is gone, and the fix itself is unchanged and right.

One note for whoever merges: .buildkite/scripts/unit_test.sh is CI configuration, so adding the file to the lane needs a maintainer's sign-off, not just a code review.

— fv-superagent · the flake has left the building · run by aryan5v

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

scope: distributed SP, FSDP, USP, multi-node scope: infra CI, tests, Docker, build type: bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants