Skip to content

fix: stage icclrun wrappers for remote MPI launches - #56

Merged
Ziminli merged 2 commits into
masterfrom
fix/stage-icclrun-wrapper-for-remote-mpi
Aug 10, 2026
Merged

fix: stage icclrun wrappers for remote MPI launches#56
Ziminli merged 2 commits into
masterfrom
fix/stage-icclrun-wrapper-for-remote-mpi

Conversation

@Ziminli

@Ziminli Ziminli commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR updates the shared MPI launcher path in scripts/backends/mpi_base.py so generated icclrun wrapper scripts are copied to node-local /tmp whenever a launch includes remote MPI nodes. This avoids executing the generated wrapper directly from shared storage such as NFS, which can fail with Text file busy while OpenMPI starts remote ranks.

Changes

  • MPI launcher staging
    • Extends the existing wrapper staging condition from node-specific dir configurations to any launch that includes at least one remote MPI node.
    • Keeps the existing node-specific dir staging behavior and single-node local behavior, while documenting why staging is needed for both node-specific paths and remote shared-storage launches.

Platform and Backend Affected

Platform

Note: Shared MPI launcher behavior can affect all platforms when launched through icclrun, however, launch/build system is independent of platforms.

  • CPU
  • NVIDIA GPU
  • Iluvatar GPU
  • MetaX GPU
  • Moore Threads GPU
  • Cambricon MLU
  • HYGON DCU

Backend

  • OpenMPI
  • MPICH
  • NCCL
  • MCCL

Performance Impact

  • No performance impact
  • Performance improved
  • Performance regression possible

N/A for benchmark results. The change adds a small wrapper copy/scp step during MPI process launch setup only; it does not affect collective data paths.

Known Issues & Future Work

  • The generated wrapper and hostfile names are still fixed per checkout. Concurrent icclrun launches from the same common_dir can still race by overwriting build/run_wrapper.sh, hosts_ompi.txt, or the staged /tmp/infiniccl_run_wrapper.sh path. A future follow-up could introduce unique per-launch artifact names plus cleanup after launch, but that is intentionally outside this minimal fix.

Test Results

Test Involved Platform

  • CPU
  • NVIDIA GPU
  • Iluvatar GPU
  • MetaX GPU
  • Moore Threads GPU
  • Cambricon MLU
  • HYGON DCU

Test Involved Backend

  • OpenMPI
  • MPICH
  • NCCL
  • MCCL

HYGON 2 nodes with OpenMPI:
mpi_all_gather.log
mpi_all_reduce.log
mpi_all_to_all.log
mpi_broadcast.log
mpi_gather.log
mpi_reduce.log
mpi_reduce_scatter.log
mpi_scatter.log
mpi_send_recv.log

Heterogeneous Cluster (NV+MetaX+Moore with OpenMPI):
mpi_gather.log
mpi_reduce.log
mpi_reduce_scatter.log
mpi_scatter.log
mpi_send_recv.log
mpi_all_gather.log
mpi_all_reduce.log
mpi_all_to_all.log
mpi_broadcast.log


Checklist

Every contributor must verify every item below before requesting
review. Tick each box only after the check has actually been performed —
do not tick speculatively. If an item truly does not apply, replace the
checkbox with N/A and briefly explain why in an inline comment.

Title, Branch, and Commits

  • PR title follows Conventional Commits (e.g. feat: …, fix(nccl): …).
  • Branch name follows <type>/xxx-yyyy-zzzz where <type> matches the PR title's Conventional Commits type and words are joined with hyphens (see CONTRIBUTING.md §Branches).
  • Each commit message follows Conventional Commits.
  • Small PR is a single squashable commit; or, for a large PR, every commit is meaningful, well-formed, and independently reviewable (see CONTRIBUTING.md §Pull Requests).
  • No stray merge commits from master — the branch is rebased cleanly on top of the current master.
  • No fixup! / squash! / wip commits remain.

Scope and Design

  • Changes are minimal — no unrelated modifications were introduced (CONTRIBUTING.md §Code/General).
  • No dead code, commented-out blocks, debug prints, printf/std::cout/print(...) left behind, or TODO without an owner and issue link.
  • No unrelated formatting churn that would obscure the diff.
  • N/A- Public API changes (if any) are intentional, documented, and reflected in affected callers/tests.

General Code Hygiene

  • The code is self-explanatory; comments were added only where the intent or rationale is non-obvious (CONTRIBUTING.md §Code/General).
  • Every modified or added file ends with a single trailing newline (CONTRIBUTING.md §Code/General).
  • No trailing whitespace, inconsistent indentation, or mixed formatting styles remain.
  • Identifiers referenced in comments or error messages are wrapped in Markdown backticks (e.g. the `AllReduce` implementation) (CONTRIBUTING.md §Code/General).
  • All comments and error messages are in English (CONTRIBUTING.md §Code/General).
  • Comments and error messages are complete sentences — capitalized first letter, terminal punctuation — unless the language/framework convention says otherwise (CONTRIBUTING.md §Code/General; §Python).

C++ Specific (if C++ files changed)

  • N/A - No C++ files changed.

Python Specific (if Python files changed)

  • Code is PEP 8 compliant; ruff check passes cleanly on CI (see .github/workflows/ruff.yml).
  • ruff format --check passes cleanly — if not, run ruff format and commit the result.
  • Comments are complete English sentences, starting with a capital letter and ending with punctuation; Markdown backticks are used for code references (CONTRIBUTING.md §Python).
  • N/A - Framework-specific conventions (e.g. lowercase pytest.skip messages without terminal period) are honored where applicable (CONTRIBUTING.md §Python).
  • No blank line between the function signature and the body when there is no docstring or comment (CONTRIBUTING.md §Python).
  • A blank line is present before and after if, for, and similar control-flow statements (CONTRIBUTING.md §Python).
  • A blank line appears before each return, except when it directly follows a control-flow statement (CONTRIBUTING.md §Python).
  • N/A - Docstrings (if any) follow PEP 257 (CONTRIBUTING.md §Python).
  • N/A - Type hints are added / kept consistent with the surrounding code.

Testing

  • All applicable example programs have been built and tested successfully on at least one supported heterogeneous cluster setup.

Build, CI, and Tooling

  • N/A - New backends or devices have been added to auto-detection in CMakeLists.txt under if(AUTO_DETECT_DEVICES) or to if(AUTO_DETECT_BACKENDS) if applicable.
  • Both CI workflows (clang-format.yml, ruff.yml) are green locally (or expected to be green on CI).

Documentation

  • N/A - README.md, CONTRIBUTING.md, or inline docs updated when behavior, build flags, or developer workflow changed.
  • N/A - Any user-visible breaking change is called out explicitly under "Summary" and in the commit/PR title with a ! or BREAKING CHANGE: footer.

Security and Safety

  • No secrets, access tokens, internal URLs, customer data, or personal hardware identifiers have been committed.
  • N/A - Third-party code is license-compatible and attributed.
  • N/A - No unsafe pointer arithmetic, uninitialized reads, or missing bounds checks were introduced.

@Ziminli Ziminli self-assigned this Aug 10, 2026
@Ziminli
Ziminli merged commit ef4045a into master Aug 10, 2026
2 checks passed
@Ziminli
Ziminli deleted the fix/stage-icclrun-wrapper-for-remote-mpi branch August 10, 2026 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant