-
Notifications
You must be signed in to change notification settings - Fork 680
Move seed from utils to training #1460
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1460
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 5a1e4f0 with merge base 069bc4d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1460 +/- ##
==========================================
+ Coverage 69.80% 72.02% +2.22%
==========================================
Files 272 272
Lines 13053 13053
==========================================
+ Hits 9111 9401 +290
+ Misses 3942 3652 -290 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be one of those rare cases in which it belongs actually in a common utils, rather than training.
My reasoning here is that we need this set_seed for generation and eval, as well, which are not training. Therefore, it would be confusing to import from trainiing.
A few of the utils APIs fall under this category, like set_device, set_dtype, etc. But I don't have strong opinions either way, if we leave set_seed in utils then I can close this PR. |
Yeah I'd prefer to leave those in utils or rename to common or something. not sure. |
There's |
Nah, I don't think these should live nested in modules/. Let's keep it where it is for now and we can discuss if we need a more descriptive name than utils. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not happy about it, but unblocking for now :/
Context
What is the purpose of this PR? Is it to
Addresses #1441.
Changelog
What are the changes made in this PR?
Test plan
Please make sure to do each of the following if applicable to your PR. (If you're not sure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.)
pre-commit install
)pytest tests
pytest tests -m integration_test
UX
If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Example of docstring: https://github.com/pytorch/torchtune/blob/6a7951f1cdd0b56a9746ef5935106989415f50e3/torchtune/modules/vision_transformer.py#L285
Example in our docs: https://pytorch.org/torchtune/main/tutorials/qat_finetune.html#applying-qat-to-llama3-models