Skip to content

Call destroy_process_group() at the end of the trainer pipelines#9726

Open
gusario wants to merge 2 commits into
modelscope:mainfrom
gusario:patch-2
Open

Call destroy_process_group() at the end of the trainer pipelines#9726
gusario wants to merge 2 commits into
modelscope:mainfrom
gusario:patch-2

Conversation

@gusario

@gusario gusario commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

same treatment as megatron path

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

SwiftPipeline.main() calls self.run() and returns without destroying the process
group, so every distributed run on the HF-trainer backend leaks it. PyTorch warns once per
rank at interpreter shutdown:

[rank24]:[W710 04:47:48.096148657 ProcessGroupNCCL.cpp:1575] Warning: WARNING:
destroy_process_group() was not called before program exit, which can leak resources.
For more info, please see https://pytorch.org/docs/stable/distributed.html#shutdown
(function operator())

On a 32-rank GRPO run this fires on every rank, on every job.

The Megatron backend already does this — swift/megatron/pipelines/train/sft.py:90-92:

# Exceptions may cause the process to hang, preventing the exception from being propagated.
# Therefore, destroy_process_group() should not be placed inside the finally block.
if dist.is_initialized():
    dist.destroy_process_group()
return self.train_msg

This PR mirrors that for the trainer pipelines, using the same guard, the same comment,
and the same placement.

Experiment results

32-rank GRPO (4 nodes × 8×H100, NCCL over InfiniBand), before and after the patch:

before after
ranks emitting destroy_process_group() was not called 31 of 32 0

(Rank 0 is absent from the "before" count because it died in an unrelated exit-time
segfault before its ProcessGroupNCCL destructor ran. Ranks 1-31 all warn.)

same treatment as megatron path

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request modifies the training pipeline in swift/pipelines/train/sft.py to destroy the distributed process group after training completes, preventing processes from hanging on exceptions. However, the dist module is not imported in this file, which will cause a NameError at runtime. The reviewer suggested importing torch.distributed as dist to resolve this issue.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread swift/pipelines/train/sft.py
@gusario

gusario commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request imports torch.distributed and updates the run method in swift/pipelines/train/sft.py to destroy the distributed process group after training completes if it was initialized. This prevents processes from hanging when exceptions occur. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

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