Remove message of checking apex.amp module and add tests for features of gradient accumulation/mixed precision training
#46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original propose of that message is to let users know gradient accumulation and mixed precision training is supported but `apex` is required. With an attention brought up by issue davidtvs#45, the following things are confirmed: - Gradient accumulation can still work properly without `apex.amp`. And that's why it would fall back on normal `loss.backward()` when `apex.amp` is not available or `amp.initialize()` wasn't called. - When mixed precision training is required, that is to say model and optimizer are wrapped by `amp.initialize()`, `amp.scale_loss()` will be adopted automatically in current implementation. Therefore, it seems that message of checking `apex.amp` module is not necessary anymore.
This mistake made batch size of every data loader become the default value: 1. Though it does not affect the correctness of all test case, it still needs to be corrected. However, `batch_size` of a `DataLoader` cannot be modified after it is initialized. Therefore, we can only determine it while generating tasks for test, and that's why `batch_size` and `steps` is moved to the signature of `__init__` of each `Task`.
This functionality was not added before, and it made all tests run on CPU even if the pytest argument `--cpu_only` is not specified.
davidtvs
reviewed
Jun 6, 2020
tests/test_lr_finder.py
Outdated
| @@ -4,6 +4,14 @@ | |||
| import task as mod_task | |||
|
|
|||
|
|
|||
| try: | |||
| import apex | |||
There was a problem hiding this comment.
I think this line can be changed to from apex import amp and we can then remove the local imports from the functions
There was a problem hiding this comment.
Yeah, I'll fix it.
tests/test_lr_finder.py
Outdated
| reason="`apex` module and gpu is required to run this test." | ||
| ) | ||
| def test_gradient_accumulation_with_apex_amp(self, mocker): | ||
| from apex import amp |
There was a problem hiding this comment.
Remove line (see comment about import)
tests/test_lr_finder.py
Outdated
| ) | ||
| class TestMixedPrecision: | ||
| def test_mixed_precision(self, mocker): | ||
| from apex import amp |
There was a problem hiding this comment.
Remove line (see comment about import)
davidtvs
requested changes
Jun 6, 2020
|
/black-check |
|
/flake8-lint |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Lintly has detected code quality issues in this pull request.
tests/test_lr_finder.py
Outdated
| @@ -4,6 +4,14 @@ | |||
| import task as mod_task | |||
|
|
|||
|
|
|||
| try: | |||
| import apex | |||
There was a problem hiding this comment.
F401: 'apex' imported but unused
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a fix for issue #45 with some new test cases for those features implemented in PR #9.
A quick summary for this PR:
To enable mixed precision training, please install apex...is removed. (solved in commit 227fc53)batch_sizewas not passed intoDataLoader, so that those data loaders in test cases were working with the default valuebatch_size=1before. Though it does not affect the test correctness, it still needs to be corrected. (solved in commit 1c549ec)task.__init__(). Therefore, all tests were running on CPU even if the pytest argument--cpu_onlyis not specified. (solved in commit c854714)Note that there is a new dependency
pytest-mockadded for new test cases.