Skip to content

Pytorch ddp - #193

Merged
s-sajid-ali merged 17 commits into
mainfrom
pytorch_ddp
Nov 4, 2025
Merged

Pytorch ddp#193
s-sajid-ali merged 17 commits into
mainfrom
pytorch_ddp

Conversation

@RobJY

@RobJY RobJY commented Oct 9, 2025

Copy link
Copy Markdown
Contributor

I found a README with this tutorial in it with the other code examples, so I modified it to work with Greene using our overlay files. I see now that it's part of a repo from Princeton. Maybe it would be better to make a fork of the original repo? This would make it clear where it comes from and why we are modifying it. We could then make markdown versions of the other 4 PyTorch tutorials in the repo as well.

How does that sound?

@RobJY
RobJY requested review from mdweisner and s-sajid-ali October 9, 2025 20:42
@RobJY RobJY self-assigned this Oct 9, 2025
@github-actions

github-actions Bot commented Oct 9, 2025

Copy link
Copy Markdown
Contributor
PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-11-04 21:23 UTC

@s-sajid-ali

s-sajid-ali commented Oct 10, 2025

Copy link
Copy Markdown
Member

Maybe it would be better to make a fork of the original repo?

Sure. Or if you're only using a handful of scripts from there they can go into https://github.com/NYU-RTS/rts-docs-examples/ as well.

This PR looks great! The only change I'd request is to fill out the intro page, we currently have two empty intro pages:

  • docs/hpc/ml_ai_hpc/intro/
  • docs/hpc/ml_ai_hpc/pytorch_intro/

Unless there is a need for both, should we remove the duplicate?

@RobJY

RobJY commented Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the review @s-sajid-ali. Great points.
I was thinking the pytorch intro would be the single GPU example from the Princeton tutorial. I'll get to work on adding it and we can include them both in this PR.
I'm not sure what to put for the general intro to the section. Please let me know if you have any suggestions.

@RobJY

RobJY commented Nov 4, 2025

Copy link
Copy Markdown
Contributor Author

I added a first pass on the intro page and the simple single GPU PyTorch example. Please let me know if you have any suggestions @s-sajid-ali. The main intro page needs more content, but I'm not sure what to add.

Reading through it I wonder if the ML and AI pages should be separated into their own sections. What do you think?

I can remove the empty pages for pytorch_lightning, pytorch_fsdp and tensorflow if you'd like to merge this soon. Or put a 'coming soon' message in those pages as placeholders.

@s-sajid-ali

Copy link
Copy Markdown
Member

This looks great!

Reading through it I wonder if the ML and AI pages should be separated into their own sections. What do you think?

By the workflow stack, they're all PyTorch at the end of the day! Right now only the two LLM pages are GenAI right now. If that number grows, we can give GenAI it's own section.

Merge soon is better, we can always add more material in follow up PRs!

@RobJY

RobJY commented Nov 4, 2025

Copy link
Copy Markdown
Contributor Author

sounds good @s-sajid-ali . I've removed the placeholder pages, so I think it's ready to go now. Please let me know if you see anything that needs to be changed. Thanks!

Clarify the usage of --ntasks-per-node in Slurm scripts.
@s-sajid-ali
s-sajid-ali merged commit d34280a into main Nov 4, 2025
5 checks passed
@s-sajid-ali
s-sajid-ali deleted the pytorch_ddp branch November 4, 2025 21:23
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.

2 participants