Increase coverage of trainers #109

adamjstewart · 2021-09-07T02:00:32Z

Since we no longer run integration tests on main, our trainer modules are currently the least well-tested code. This PR attempts to add unit tests for these trainers. I'm a bit stuck at the moment, so opening this up for feedback on better ways to test this code while I focus on more important things.

calebrob6 · 2021-10-16T21:24:10Z

@adamjstewart it doesn't look like Lightning is made for you to call train_step, etc. outside of a Trainer. For example, we use self.log everywhere in the trainers, however this is a method that is created when you are using a Trainer object with .fit()

calebrob6 · 2021-10-17T00:27:37Z

Alright, debugged this and got a pattern for getting 100% test coverage + 100% coverage in SEN12MS and Cyclone trainers

calebrob6 · 2021-10-17T04:58:07Z

Alright @adamjstewart, summary of what happened here.

I wrote tests for the Cyclone, SEN12MS, LandCoverAI, So2Sat, and RESISC45 trainers. In most cases this involved some cleaning up of the trainer code to make everything a little more homogenous.

In RESISC45 specifically:

I made the test dataset size larger as we need to split it into train/val/test with batch size>=2
I skip the trainer tests on windows because of the unrar problem

Things that are missing for this PR (can you take these?):

Tests for Chesapeake and BYOL (and NAIPChesapeake if you want to keep that?)
An appropriate way to test the add_figure call we are sometimes using to render predictions on TensorBoard in the validation_step (e.g. https://github.com/microsoft/torchgeo/blob/tests/trainers/torchgeo/trainers/landcoverai.py#L196)
An appropriate way to test loading a checkpoint file within the So2Sat trainer (https://github.com/microsoft/torchgeo/blob/tests/trainers/torchgeo/trainers/so2sat.py#L96)

adamjstewart · 2021-10-19T19:43:47Z

and NAIPChesapeake if you want to keep that?

Up to you. I believe the original intent was to use this to generate pre-trained model weights.

adamjstewart · 2021-10-22T22:01:00Z

@calebrob6 can you take a look at my last commit? As far as I can tell, the tests are correct, but the trainer task itself is broken? I tried all possible combinations of model and loss and most of them break in one way or another.

calebrob6 · 2021-10-22T22:18:31Z

~~What doesn't work?~~

Looking through the test output it seems that the label mask used for training has more values than expected. Also, the training data patches might be very small. How did you generate the test files?

adamjstewart · 2021-10-22T22:41:08Z

Ah, that might explain it. I generated 512x512 files with the same number of bands and random ints. So it sounds like I need to be careful with the range of those ints in the label files.

adamjstewart · 2021-10-26T04:25:24Z

Okay, this should be good to go now! Once this is merged, TorchGeo will have 100% test coverage.

The tests themselves should be fine, but please carefully review all changes to torchgeo itself to make sure I didn't break anything in the process of getting the tests to work.

adamjstewart · 2021-10-26T04:26:25Z

tests/trainers/conftest.py

@@ -0,0 +1,44 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.


conftest.py is a special file that pytest looks for. It allows us to share fixtures across multiple files in a directory.

adamjstewart · 2021-10-26T04:27:06Z

tests/trainers/test_byol.py

+from .test_utils import mocked_log
+
+
+@pytest.fixture(scope="module")


scope="module" means that this fixture is only instantiated a single time instead of every time it gets requested. This speeds things up a bit, although the trainer tests are still very slow.

adamjstewart · 2021-10-26T04:28:31Z

tests/trainers/test_sen12ms.py

+    scope="module", params=[("all", 15), ("s1", 2), ("s2-all", 13), ("s2-reduced", 6)]
+)
+def bands(request: SubRequest) -> Tuple[str, int]:
+    return cast(Tuple[str, int], request.param)


Found this super clever way of syncing two separate fixtures (datamodule and config) to share the same values.

adamjstewart · 2021-10-26T04:29:14Z

tests/trainers/test_utils.py



 def test_extract_encoder_unsupported_model(tmp_path: Path) -> None:
    checkpoint = {"hyper_parameters": {"some_unsupported_model": "resnet18"}}
    path = os.path.join(str(tmp_path), "dummy.ckpt")
    torch.save(checkpoint, path)
-    err = """Unknown checkpoint task. Only encoder or classification_model"""
-    """extraction is supported"""


These tests weren't actually working properly. The second line doesn't get appended to the string, so it was only ever checking for the first half.

adamjstewart · 2021-10-26T04:31:26Z

torchgeo/trainers/utils.py

@@ -80,15 +80,15 @@ def load_state_dict(model: Module, state_dict: Dict[str, Tensor]) -> Module:

    if in_channels != expected_in_channels:
        warnings.warn(
-            f"""input channels {in_channels} != input channels in pretrained"""
-            """model {expected_in_channels}. Overriding with new input channels"""


These second lines were missing f-string processing and a space between words

calebrob6

Some other things:

conf/naipchesapeake.conf needs to be updated
An FYI, there are currently 578 tests for the trainers and it takes 10 minutes to run on my VM.
I'm guessing a lot of test time is spent running Conv2ds on the CPU. If so, the Chesapeake tests could be sped up with smaller patch sizes. I'm wondering if the fake LandCoverAI data could be reduced in size too (e.g. 512 --> 64 would be way faster).
~~Have you tried to run the naipchesapeake trainer?~~ Edit: I'm able to run the naipchesapeake trainer in a notebook

tests/data/chesapeake/cvpr/spatial_index.geojson

tests/trainers/test_naipchesapeake.py

torchgeo/trainers/naipchesapeake.py

adamjstewart · 2021-10-26T14:19:35Z

conf/naipchesapeake.conf needs to be updated

Updated in what way?

adamjstewart · 2021-10-26T14:45:27Z

I'm guessing a lot of test time is spent running Conv2ds on the CPU.

After the last commit, most time is actually spent initializing the models:

$ pyinstrument $(which pytest) tests/trainers/test_chesapeake.py 
...
117.755 <module>  <string>:1
   [4 frames hidden]  <string>, runpy
      117.755 _run_code  runpy.py:64
      └─ 117.755 <module>  pytest:3
         └─ 117.561 console_main  _pytest/config/__init__.py:178
               [3486 frames hidden]  _pytest, pluggy, typing, inspect, <bu...
                  64.225 call_fixture_func  _pytest/fixtures.py:916
                  ├─ 62.387 task  tests/trainers/test_chesapeake.py:55
                  │  └─ 62.384 __init__  torchgeo/trainers/chesapeake.py:100
                  │     └─ 61.314 config_task  torchgeo/trainers/chesapeake.py:59
                  │        ├─ 31.118 __init__  segmentation_models_pytorch/unet/model.py:50
                  │        │     [1183 frames hidden]  segmentation_models_pytorch, torchvis...
                  │        └─ 29.098 __init__  segmentation_models_pytorch/deeplabv3/model.py:123
                  │              [1361 frames hidden]  segmentation_models_pytorch, torchvis...
                  └─ 1.239 datamodule  tests/trainers/test_chesapeake.py:19
                     └─ 1.238 wrapped_fn  pytorch_lightning/core/datamodule.py:393
                  1.558 call_fixture_func  _pytest/fixtures.py:916
                  └─ 1.553 config  tests/trainers/test_chesapeake.py:38
                     └─ 1.482 load  omegaconf/omegaconf.py:178
                           [1343 frames hidden]  omegaconf, yaml, <built-in>, abc, cod...
                  38.158 pytest_pyfunc_call  _pytest/python.py:176
                  ├─ 13.584 test_validation  tests/trainers/test_chesapeake.py:77
                  │  └─ 12.254 validation_step  torchgeo/trainers/chesapeake.py:174
                  │     ├─ 7.470 wrapper  matplotlib/_api/deprecation.py:459
                  │     │     [6280 frames hidden]  matplotlib, <string>, <built-in>, con...
                  │     └─ 4.062 forward  torchgeo/trainers/chesapeake.py:128
                  │        └─ 4.062 _call_impl  torch/nn/modules/module.py:1045
                  │              [239 frames hidden]  torch, segmentation_models_pytorch, t...
                  │                 2.182 forward  torchgeo/models/fcn.py:61
                  │                 └─ 2.182 _call_impl  torch/nn/modules/module.py:1045
                  │                       [17 frames hidden]  torch, <built-in>
                  ├─ 11.427 test_test  tests/trainers/test_chesapeake.py:84
                  │  └─ 10.160 test_step  torchgeo/trainers/chesapeake.py:247
                  │     └─ 9.354 forward  torchgeo/trainers/chesapeake.py:128
                  │        └─ 9.354 _call_impl  torch/nn/modules/module.py:1045
                  │              [237 frames hidden]  torch, segmentation_models_pytorch, t...
                  │                 7.211 forward  torchgeo/models/fcn.py:61
                  │                 └─ 7.211 _call_impl  torch/nn/modules/module.py:1045
                  │                       [15 frames hidden]  torch, <built-in>
...

adamjstewart · 2021-10-26T15:18:44Z

The last couple of commits have reduced total pytest time from 8m 26s to 2m 24s (Python 3.9, Ubuntu). Still more than I would like, but you can always test only the file you are working on instead of running all tests. There's likely additional places we can speed things up if we need to, such as smaller raster files and parallel testing.

adamjstewart · 2021-10-26T15:58:29Z

Would like to get rid of these warnings:

tests/trainers/test_byol.py::TestBYOLTask::test_training[resnet18]
tests/trainers/test_byol.py::TestBYOLTask::test_training[resnet50]
tests/trainers/test_byol.py::TestBYOLTask::test_validation[resnet18]
tests/trainers/test_byol.py::TestBYOLTask::test_validation[resnet50]
  /opt/hostedtoolcache/Python/3.9.7/x64/lib/python3.9/site-packages/torch/nn/functional.py:3631: UserWarning: Default upsampling behavior when mode=bilinear is changed to align_corners=False since 0.4.0. Please specify align_corners=True if the old behavior is desired. See the documentation of nn.Upsample for details.

Any idea if this is in code we can control or if it's internal to PyTorch/torchvision/Kornia?

isaaccorley · 2021-10-26T16:44:28Z

I believe this happens in the BYOL trainer in the augmentations when using K.Resize and K.RandomResizedCrop when not explicitly defining align_corners=True. But the warning comes from the nn.Upsample which is under the hood so it may not suppress it but it's worth a try.

adamjstewart · 2021-10-26T18:33:46Z

Alright, all warnings have been silenced and the tests are 4x faster now. I think this is ready for a second round of review.

calebrob6 · 2021-10-26T19:57:24Z

Updated in what way?

It doesn't follow how the other config files are arranged and would not work if passed to train.py. This can probably just be deleted.

calebrob6 · 2021-10-26T19:58:46Z

Nicely done!!

calebrob6 · 2021-10-26T20:00:19Z

I'm guessing a lot of test time is spent running Conv2ds on the CPU.

After the last commit, most time is actually spent initializing the models:

$ pyinstrument $(which pytest) tests/trainers/test_chesapeake.py 
...
117.755 <module>  <string>:1
   [4 frames hidden]  <string>, runpy
      117.755 _run_code  runpy.py:64
      └─ 117.755 <module>  pytest:3
         └─ 117.561 console_main  _pytest/config/__init__.py:178
               [3486 frames hidden]  _pytest, pluggy, typing, inspect, <bu...
                  64.225 call_fixture_func  _pytest/fixtures.py:916
                  ├─ 62.387 task  tests/trainers/test_chesapeake.py:55
                  │  └─ 62.384 __init__  torchgeo/trainers/chesapeake.py:100
                  │     └─ 61.314 config_task  torchgeo/trainers/chesapeake.py:59
                  │        ├─ 31.118 __init__  segmentation_models_pytorch/unet/model.py:50
                  │        │     [1183 frames hidden]  segmentation_models_pytorch, torchvis...
                  │        └─ 29.098 __init__  segmentation_models_pytorch/deeplabv3/model.py:123
                  │              [1361 frames hidden]  segmentation_models_pytorch, torchvis...
                  └─ 1.239 datamodule  tests/trainers/test_chesapeake.py:19
                     └─ 1.238 wrapped_fn  pytorch_lightning/core/datamodule.py:393
                  1.558 call_fixture_func  _pytest/fixtures.py:916
                  └─ 1.553 config  tests/trainers/test_chesapeake.py:38
                     └─ 1.482 load  omegaconf/omegaconf.py:178
                           [1343 frames hidden]  omegaconf, yaml, <built-in>, abc, cod...
                  38.158 pytest_pyfunc_call  _pytest/python.py:176
                  ├─ 13.584 test_validation  tests/trainers/test_chesapeake.py:77
                  │  └─ 12.254 validation_step  torchgeo/trainers/chesapeake.py:174
                  │     ├─ 7.470 wrapper  matplotlib/_api/deprecation.py:459
                  │     │     [6280 frames hidden]  matplotlib, <string>, <built-in>, con...
                  │     └─ 4.062 forward  torchgeo/trainers/chesapeake.py:128
                  │        └─ 4.062 _call_impl  torch/nn/modules/module.py:1045
                  │              [239 frames hidden]  torch, segmentation_models_pytorch, t...
                  │                 2.182 forward  torchgeo/models/fcn.py:61
                  │                 └─ 2.182 _call_impl  torch/nn/modules/module.py:1045
                  │                       [17 frames hidden]  torch, <built-in>
                  ├─ 11.427 test_test  tests/trainers/test_chesapeake.py:84
                  │  └─ 10.160 test_step  torchgeo/trainers/chesapeake.py:247
                  │     └─ 9.354 forward  torchgeo/trainers/chesapeake.py:128
                  │        └─ 9.354 _call_impl  torch/nn/modules/module.py:1045
                  │              [237 frames hidden]  torch, segmentation_models_pytorch, t...
                  │                 7.211 forward  torchgeo/models/fcn.py:61
                  │                 └─ 7.211 _call_impl  torch/nn/modules/module.py:1045
                  │                       [15 frames hidden]  torch, <built-in>
...

How much did reducing the patch sizes speed everything up?

adamjstewart · 2021-10-26T20:03:52Z

I think reducing patch size was 2x and reducing params was another 2x.

calebrob6 · 2021-10-26T20:52:13Z

Good to know, thanks! Everything here looks good to me

* Increase coverage of trainers * Actually make the tests work * Updated Cyclone trainer * Style fix in cyclone tests * Fixing landcoverai trainer * Moving mock log to utils * Fixing the RESISC45 trainer and related * Skip RESISC45 trainer tests if Windows * Removing some stupid docstrings from the RESISC45 trainer * Adding So2Sat trainer tests * isort * Adding RESISC45 test data * Use os.path.join for paths * Remove unused import * Add tests for ChesapeakeCVPR trainer * mypy fixes * Fix most Chesapeake tests * Fixed test batching issue in the test dataset * Get 100% coverage of Chesapeake trainer * use a FakeTrainer instead of pl.Trainer * Add naive BYOL trainer tests * Style fixes * Add 100% test coverage for BYOL trainer * Get 100% coverage for LandCover.ai trainer * Simplify tests * Add tests for NAIP + Chesapeake trainer * Fix tests * Add tests for checkpoint loading * Reorganize fixtures and specify scope * Fix various test bugs * Mypy fixes * Reduce patch sizes * Test fewer possible combinations of params * Prevent warnings in tests * Restore missing line of coverage in So2Sat trainer * Silence resampling warning * Ignore difference in output classes Co-authored-by: Caleb Robinson <calebrob6@gmail.com>

adamjstewart added testing Continuous integration testing trainers PyTorch Lightning trainers labels Sep 7, 2021

adamjstewart mentioned this pull request Sep 9, 2021

Overhaul of train.py and adding Chesapeake CVPR trainer #103

Merged

adamjstewart mentioned this pull request Oct 10, 2021

Add RESISC45 Trainer #179

Merged

calebrob6 force-pushed the tests/trainers branch from f75dafa to c14173c Compare October 16, 2021 21:05

calebrob6 marked this pull request as ready for review October 17, 2021 04:12

calebrob6 force-pushed the tests/trainers branch from 03d8752 to 7d71dec Compare October 17, 2021 05:02

adamjstewart and others added 15 commits October 22, 2021 16:58

Increase coverage of trainers

1154211

Actually make the tests work

bc8b56a

Updated Cyclone trainer

bf834ed

Style fix in cyclone tests

a063c90

Fixing landcoverai trainer

c4afec8

Moving mock log to utils

9349a12

Fixing the RESISC45 trainer and related

174e51f

Skip RESISC45 trainer tests if Windows

bcefdf0

Removing some stupid docstrings from the RESISC45 trainer

3551c77

Adding So2Sat trainer tests

3ff1440

isort

78cb3ff

Adding RESISC45 test data

d22dfda

Use os.path.join for paths

5a2abee

Remove unused import

af38639

Add tests for ChesapeakeCVPR trainer

f00b4e5

adamjstewart force-pushed the tests/trainers branch from a60b5f4 to f00b4e5 Compare October 22, 2021 21:58

adamjstewart requested a review from isaaccorley October 26, 2021 04:23

adamjstewart commented Oct 26, 2021

View reviewed changes

calebrob6 requested changes Oct 26, 2021

View reviewed changes

tests/data/chesapeake/cvpr/spatial_index.geojson Show resolved Hide resolved

tests/trainers/test_naipchesapeake.py Outdated Show resolved Hide resolved

torchgeo/trainers/naipchesapeake.py Show resolved Hide resolved

Reduce patch sizes

250743c

Test fewer possible combinations of params

67ad878

Prevent warnings in tests

1f4d5c2

Restore missing line of coverage in So2Sat trainer

331ebc5

adamjstewart added 2 commits October 26, 2021 11:48

Silence resampling warning

6c0a83f

Ignore difference in output classes

163c220

calebrob6 approved these changes Oct 26, 2021

View reviewed changes

adamjstewart merged commit 10ff8aa into main Oct 26, 2021

adamjstewart deleted the tests/trainers branch October 26, 2021 21:17

adamjstewart added this to the 0.1.0 milestone Nov 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase coverage of trainers #109

Increase coverage of trainers #109

adamjstewart commented Sep 7, 2021

calebrob6 commented Oct 16, 2021 •

edited

calebrob6 commented Oct 17, 2021

calebrob6 commented Oct 17, 2021 •

edited

adamjstewart commented Oct 19, 2021

adamjstewart commented Oct 22, 2021

calebrob6 commented Oct 22, 2021 •

edited

adamjstewart commented Oct 22, 2021

adamjstewart commented Oct 26, 2021

adamjstewart Oct 26, 2021

adamjstewart Oct 26, 2021

adamjstewart Oct 26, 2021

adamjstewart Oct 26, 2021 •

edited

adamjstewart Oct 26, 2021

calebrob6 left a comment •

edited

adamjstewart commented Oct 26, 2021

adamjstewart commented Oct 26, 2021 •

edited

adamjstewart commented Oct 26, 2021

adamjstewart commented Oct 26, 2021 •

edited

isaaccorley commented Oct 26, 2021

adamjstewart commented Oct 26, 2021

calebrob6 commented Oct 26, 2021 •

edited

calebrob6 commented Oct 26, 2021

calebrob6 commented Oct 26, 2021

adamjstewart commented Oct 26, 2021

calebrob6 commented Oct 26, 2021

		@@ -0,0 +1,44 @@
		# Copyright (c) Microsoft Corporation. All rights reserved.

		from .test_utils import mocked_log


		@pytest.fixture(scope="module")

Increase coverage of trainers #109

Increase coverage of trainers #109

Conversation

adamjstewart commented Sep 7, 2021

calebrob6 commented Oct 16, 2021 • edited

calebrob6 commented Oct 17, 2021

calebrob6 commented Oct 17, 2021 • edited

adamjstewart commented Oct 19, 2021

adamjstewart commented Oct 22, 2021

calebrob6 commented Oct 22, 2021 • edited

adamjstewart commented Oct 22, 2021

adamjstewart commented Oct 26, 2021

adamjstewart Oct 26, 2021

Choose a reason for hiding this comment

adamjstewart Oct 26, 2021

Choose a reason for hiding this comment

adamjstewart Oct 26, 2021

Choose a reason for hiding this comment

adamjstewart Oct 26, 2021 • edited

Choose a reason for hiding this comment

adamjstewart Oct 26, 2021

Choose a reason for hiding this comment

calebrob6 left a comment • edited

Choose a reason for hiding this comment

adamjstewart commented Oct 26, 2021

adamjstewart commented Oct 26, 2021 • edited

adamjstewart commented Oct 26, 2021

adamjstewart commented Oct 26, 2021 • edited

isaaccorley commented Oct 26, 2021

adamjstewart commented Oct 26, 2021

calebrob6 commented Oct 26, 2021 • edited

calebrob6 commented Oct 26, 2021

calebrob6 commented Oct 26, 2021

adamjstewart commented Oct 26, 2021

calebrob6 commented Oct 26, 2021

calebrob6 commented Oct 16, 2021 •

edited

calebrob6 commented Oct 17, 2021 •

edited

calebrob6 commented Oct 22, 2021 •

edited

adamjstewart Oct 26, 2021 •

edited

calebrob6 left a comment •

edited

adamjstewart commented Oct 26, 2021 •

edited

adamjstewart commented Oct 26, 2021 •

edited

calebrob6 commented Oct 26, 2021 •

edited