-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Properly handle parent modules w/ parameters in BaseFinetuning callback
#7931
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
Properly handle parent modules w/ parameters in BaseFinetuning callback
#7931
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7931 +/- ##
=======================================
- Coverage 92% 87% -5%
=======================================
Files 203 203
Lines 13127 13127
=======================================
- Hits 12013 11395 -618
- Misses 1114 1732 +618 |
carmocca
left a comment
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 catch!
Remember to update the CHANGELOG.md file
BaseFinetuning callback
e477e51 to
d36525d
Compare
…was a superset, updated changelog
d36525d to
8b8a8cf
Compare
tchaton
left a comment
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.
Good catch ! Thanks for using the BaseFinetuning Callback and catching this bug.
…back (#7931) Co-authored-by: Daniel Dale <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>
…back (#7931) Co-authored-by: Daniel Dale <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>
…back (#7931) Co-authored-by: Daniel Dale <[email protected]> Co-authored-by: Carlos Mocholí <[email protected]> Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>
What does this PR do?
Fixes #7930
This PR fixes issue #7930 and provides relevant test coverage: test_finetuning_callback.py::test_parent_module_w_param_model
Models w/ parent modules that directly contained parameters themselves (as opposed to parameters exclusively in their constituent submodules) had their parameters ignored by the BaseFinetuning callback. The primary issue was addressed with an additional filtering condition in BaseFinetuning.flatten_modules(). To avoid passing duplicate parameters in methods that depended upon BaseFinetuning.flatten_modules, I also added recurse=False to the parameter loops in BaseFinetuning.[freeze, make_trainable, filter_params]. The new test I've added could potentially be consolidated with /tests/callbacks/test_finetuning_callback.py::test_deep_nested_model but I at least initially have broken the test out separately for clarity. As a practical example of the issue, I initially encountered this bug when using BaseFinetuning w/ deberta, specifically, the DisentangledSelfAttention parent module
I've updated the docstring of flatten_modules() to reflect the changes. All test_finetuning_callback.py tests are passing on my end.
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
YES! This is my first contribution to PL but I'm hoping to be more involved in the future. Thanks for the awesome product/work!