Skip to content

Fix/warnings stacklevel mvapich runner#7949

Closed
nathon-lee wants to merge 16 commits into
deepspeedai:masterfrom
nathon-lee:fix/warnings-stacklevel-mvapich-runner
Closed

Fix/warnings stacklevel mvapich runner#7949
nathon-lee wants to merge 16 commits into
deepspeedai:masterfrom
nathon-lee:fix/warnings-stacklevel-mvapich-runner

Conversation

@nathon-lee

Copy link
Copy Markdown
Contributor

MVAPICHRunner.backend_exists calls warnings.warn without stacklevel,
so Python reports the warning as coming from inside DeepSpeed rather than
the user's code. Adding stacklevel=2 fixes the reported call site.

No functional change.

@nathon-lee nathon-lee requested a review from loadams as a code owner April 2, 2026 14:00
@tohtana

tohtana commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Hi @nathon-lee,
Thank you for the PR~ I wonder if stacklevel=2 still doesn't point the user code. Before this PR, it isdeepspeed/launcher/multinode_runner.py but it will be deepspeed/launcher/runner.py.

@nathon-lee

Copy link
Copy Markdown
Contributor Author

Hi @nathon-lee, Thank you for the PR~ I wonder if stacklevel=2 still doesn't point the user code. Before this PR, it isdeepspeed/launcher/multinode_runner.py but it will be deepspeed/launcher/runner.py.

Hi @tohtana, you're right — stacklevel=2 still points inside DeepSpeed, and since a RuntimeError is raised right after, users won't even see the warning. This change has no real effect. Converting to draft while I reconsider. Thanks!

@nathon-lee nathon-lee marked this pull request as draft April 9, 2026 02:15
@sfc-gh-truwase

Copy link
Copy Markdown
Collaborator

@nathon-lee do you intend to keep this as draft?

@nathon-lee nathon-lee marked this pull request as ready for review June 25, 2026 13:24
@nathon-lee

Copy link
Copy Markdown
Contributor Author

Hi, @sfc-gh-truwase
Thanks for checking! I don’t plan to keep it as draft — I’ll mark it as Ready for review.
This PR only adds stacklevel=2 to warnings.warn (no functional change), so the warning points to user call sites and is easier to debug.
If this looks good to you, could you help merge it? Thank you!

@sfc-gh-truwase

Copy link
Copy Markdown
Collaborator

@nathon-lee can you clarify that your reconsideration based on @tohtana comment is to merge even if there is no functional impact?

@nathon-lee

Copy link
Copy Markdown
Contributor Author

Hi @tohtana and @sfc-gh-truwase,

Thanks for the review. After revisiting the call path, I agree with your point: stacklevel=2 does not actually point to user code in this case, so this change does not provide meaningful user-facing value.

Since the launcher also raises a RuntimeError immediately afterward, I do not think this PR is worth keeping open as-is. I will close it for now rather than keep a low-impact change under review.

Thanks again for the careful feedback. If useful, I can come back later with a more meaningful improvement to the MVAPICH error/reporting path.

@nathon-lee nathon-lee closed this Jun 28, 2026
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.

4 participants