-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
add functions to inspect model and optimizer status to trainer.py #29838
add functions to inspect model and optimizer status to trainer.py #29838
Conversation
…er group for parameters and get learning rates of param groups to trainer.py
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.
Very nice PR! Any chance you could add some basic tests in tests/trainer/test_trainer.py
for these?
Of course, I wasn't sure where to add them, but now I see that I missed that |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Great, thanks! I'll give a ✅ after, great job and very clean implementation :) |
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.
Nice - thanks for adding!
+1 for adding tests. Once those are done, you can ping for a final review and we can merge 🤗
tests/trainer/test_trainer.py
Outdated
out_features = 64 | ||
# in_features * out_features + bias | ||
expected_num_params = in_features * out_features + out_features | ||
model = nn.Sequential(nn.Linear(in_features, out_features)) |
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.
Let's perhaps make this two linear layers, one that's frozen, one that's not, so this way trainable checks if we have different optimizer groups? :)
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.
Yes, you're right, that would definitely be better :)
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.
Nice tests! Thanks for adding!
Just a small nit on the torch condition - then we're good to merge!
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
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.
Thanks! Looks good to me as well, let's fix that spacing though 😉
Co-authored-by: Zach Mueller <muellerzr@gmail.com>
Is there anything else to do? :) @amyeroberts @muellerzr |
@CKeibel Nope - merging now! |
Excellent thanks, then I'll start looking for a new issue to work on. :) |
Looking forward to the future PRs :) Thanks again for contributing to improving the library! |
What does this PR do?
Add functions
get_num_trainable_parameters
to return the number of parameters which require grad,get_learning_rates
to return the learning rates of parameter groups andget_optimizer_group
to return optimizer groups for parameters.Fixes #29016
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@amyeroberts @muellerzr