Skip to content

fix(thunder): remove duplicate _configure_launcher that dropped the external-launch guard - #2324

Merged
bhimrazy merged 2 commits into
Lightning-AI:mainfrom
Anai-Guo:fix/thunder-ddp-dup-configure-launcher
Sep 9, 2026
Merged

fix(thunder): remove duplicate _configure_launcher that dropped the external-launch guard#2324
bhimrazy merged 2 commits into
Lightning-AI:mainfrom
Anai-Guo:fix/thunder-ddp-dup-configure-launcher

Conversation

@Anai-Guo

@Anai-Guo Anai-Guo commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Problem

ThunderDDPStrategy in extensions/thunder/strategies/thunder_ddp.py defines _configure_launcher twice within the same class:

@override
def _configure_launcher(self) -> None:
    assert self.cluster_environment is not None
    if not self.cluster_environment.creates_processes_externally:   # guarded
        self._launcher = _SubprocessScriptLauncher(self.cluster_environment, self.num_processes, self.num_nodes)

@property
def process_group_backend(self) -> str | None:
    return self._process_group_backend

@override
def _configure_launcher(self) -> None:                              # duplicate, wins
    assert self.cluster_environment is not None
    self._launcher = _SubprocessScriptLauncher(self.cluster_environment, self.num_processes, self.num_nodes)

The second definition silently overrides the first and drops the if not self.cluster_environment.creates_processes_externally guard. As a result, a _SubprocessScriptLauncher is always installed — even when the processes are launched externally (e.g. torchrun, SLURM, or any environment where creates_processes_externally is True). In that case the strategy should not create a subprocess launcher, otherwise it attempts to re-spawn the worker processes itself.

Fix

Remove the unguarded duplicate, keeping the guarded version. This matches:

  • the sibling ThunderFSDPStrategy._configure_launcher in the same directory (thunder_fsdp.py), which has the single, guarded implementation, and
  • Lightning's own DDPStrategy._configure_launcher.

No behavior change when launching normally; the guard is now honored again for externally-launched processes.

🤖 Generated with Claude Code

…xternal-launch guard

ThunderDDPStrategy defined _configure_launcher twice. The second definition
silently overrode the first and omitted the
'if not self.cluster_environment.creates_processes_externally' guard, so a
_SubprocessScriptLauncher was created even when processes are launched
externally (e.g. torchrun/SLURM). Remove the unguarded duplicate, keeping the
guarded version that matches ThunderFSDPStrategy._configure_launcher and
Lightning's DDPStrategy.

@bhimrazy bhimrazy 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.

LGTM, Thanks @Anai-Guo

@bhimrazy
bhimrazy enabled auto-merge (squash) September 9, 2026 01:16
@bhimrazy
bhimrazy merged commit 18c931c into Lightning-AI:main Sep 9, 2026
17 checks passed
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.

3 participants