ContinualAI
Changes in Dynamic Modules
I tried to implement the changes that would fix bugs described in Issue #1591 and also allow users to use Dynamic modules more easily outside of avalanche strategies.
To do so, I made several major changes.
- Remove old avalanche_model_adaptation method and replace it with a new version which does things differently. Instead of iterating on all modules, it iterates only on childrens, and does so recursively in order to explore the whole hierarchy. It still iterates on the hierarchy of nn.Modules and catches DynamicModules to adapt them just like before.
- Added a method adapt() to dynamicmodule that will call the new "avalanche_model_adaptation" on itself and recursively on it's submodules
- Added an _auto_adapt attribute to all DynamicModules that triggers or not the adaptation inside the avalanche_model_adaptation recursion (defaults to True). In particular, this attribute does not prevent the user to call directly module.adapt() if he wants, however if the module needs to be adapted as a second level or more module inside this recursion, it will not get adapted. This is the part that fixes the bug described in #1591, since that way we can instantiate IncrementalClassifiers inside the MultiHeadClassifier that will not be automatically adapted but rather adapted manually by the MultiHeadClassifier only if the experience used for adaptation contains the task id related to this IncrementalClassifier.
- Made a few clarification changes in IncrementalClassifier and Multiheadclassifier by replacing occurences of adaptation() implementation by train_adaptation and eval_adaptation, where if the eval_adaptation needs to be the same than train_adaptation I just call train_adaptation inside eval_adaptation (nested adaptation() override were a bit complex to understand and could lead to confusion)
For now tests do not pass, but some of them I am not sure what behaviour they were checking and if we really want this behaviour. I will also add some more tests to check that the behaviour in #1591 is solved (my local test says it is fixed for now).
Pull Request Test Coverage Report for Build 8038239946
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 0 of 115 (100.0%) changed or added relevant lines in 6 files are covered.
- 60 unchanged lines in 1 file lost coverage.
- Overall coverage increased (+0.1%) to 64.167%
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| avalanche/training/supervised/lamaml_v2.py | 60 | 0.0% |
| <!-- | Total: | 60 |
| Totals | |
|---|---|
| Change from base Build 7933962330: | 0.1% |
| Covered Lines: | 20155 |
| Relevant Lines: | 31410 |
💛 - Coveralls
Made a few clarification changes in IncrementalClassifier and Multiheadclassifier by replacing occurences of adaptation() implementation by train_adaptation and eval_adaptation, where if the eval_adaptation needs to be the same than train_adaptation I just call train_adaptation inside eval_adaptation (nested adaptation() override were a bit complex to understand and could lead to confusion)
I think we could also remove train_adaptation and eval_adaptation and have a single method. After, you can detect train/eval mode with a single if when necessary, so it seems to be adding unnecessary complexity in the common case.
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
The advantage of the current solution is that it's backward compatible. If we don't want to rename the old method we could rename the new one as rec_adapt for recursive adaptation?
I was also thinking about a test case to check that the recursive calls don't break the #1591 fix:
model = ABasicDynamicModule(ABasicModule(MultiTaskClassifier(...)))
model.adapt(exp) # here it's calling rec_adapt from the "base" layer, not the MultiTaskClassifier. It should still work.
# assert that wrong heads are not adapted
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
The advantage of the current solution is that it's backward compatible. If we don't want to rename the old method we could rename the new one as
rec_adaptfor recursive adaptation?I was also thinking about a test case to check that the recursive calls don't break the #1591 fix:
model = ABasicDynamicModule(ABasicModule(MultiTaskClassifier(...))) model.adapt(exp) # here it's calling rec_adapt from the "base" layer, not the MultiTaskClassifier. It should still work. # assert that wrong heads are not adapted
I added a test for that at the end of test_models.DynamicModulesTest. Yes, maybe I will rename to make it clearer. Do you know how I can fix these failed Coveralls?
Made a few clarification changes in IncrementalClassifier and Multiheadclassifier by replacing occurences of adaptation() implementation by train_adaptation and eval_adaptation, where if the eval_adaptation needs to be the same than train_adaptation I just call train_adaptation inside eval_adaptation (nested adaptation() override were a bit complex to understand and could lead to confusion)
I think we could also remove
train_adaptationandeval_adaptationand have a single method. After, you can detect train/eval mode with a single if when necessary, so it seems to be adding unnecessary complexity in the common case.
Yes I agree, I don't remember why we included them in the first place
@AntonioCarta On my side everything is ok. I was thinking maybe to include an example on how to use dynamic modules outside and inside of a strategy (call to recursive_adaptation and explanation on how avalanche_model_adaptation works). Another thing is that I changed the location of avalanche_model_adaptation for now it's inside dynamic_modules.py before it was in models/utils.py. For now I import it in utils so that ppl don't have to change their imports, but it's a bit weird. The reason I put it inside dynamic_modules is that this function is needed in recursive_adaptation() method so putting it to utils was leading to annoying recursive imports.
I also just noticed that NCM is still using eval_adaptation so I will need to change that. I don't know how this has not been caught by some tests.
Ok. I will wait to merge this after release to be on the safe side.
I was thinking maybe to include an example on how to use dynamic modules outside and inside of a strategy
we already have examples of that in the notebooks. Maybe you can just update them?