Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log generation only on rank 0 for distributed #171

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Log generation only on rank 0 for distributed #171

merged 1 commit into from
Jan 10, 2024

Conversation

rohan-varma
Copy link
Member

Changelog

  • Avoids excessive log noise in the case of distributed.

Test plan

  • Run finetune:
torchrun --nnodes 1 --nproc_per_node 8 recipes/finetune_llm.py --config recipes/configs/alpaca_llama2_finetune.yaml --autocast-precision bf16 --fsdp True --batch-size 1 --run-generation 50 --optimizer AdamW

Copy link

netlify bot commented Jan 10, 2024

Deploy Preview for torchtune-preview ready!

Name Link
🔨 Latest commit ebe11a7
🔍 Latest deploy log https://app.netlify.com/sites/torchtune-preview/deploys/659f1f39307d60000865fd92
😎 Deploy Preview https://deploy-preview-171--torchtune-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 10, 2024
Copy link
Contributor

@joecummings joecummings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I like having a utility for these things (e.g. print0 here). Curious if this falls under the category of things we don't want to abstract away in the recipe though. Either way, looks good!

@joecummings
Copy link
Contributor

Personally I like having a utility for these things (e.g. print0 here). Curious if this falls under the category of things we don't want to abstract away in the recipe though. Either way, looks good!

I like this - especially b/c it might need to be repeated elsewhere.

@rohan-varma rohan-varma mentioned this pull request Jan 10, 2024
@rohan-varma
Copy link
Member Author

print0 makes a lot of sense, we can add that as a follow up: #172

@rohan-varma rohan-varma merged commit 2c36db2 into main Jan 10, 2024
15 checks passed
Comment on lines +178 to +181
if (
not torch.distributed.is_initialized()
or torch.distributed.get_rank() == 0
):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants