Test minimum version of deps #574

adamjstewart · 2022-06-12T17:03:58Z

This is the fourth PR in a series of attempts to improve the stability and robustness of TorchGeo's CI (#567, #557, #551).

With this PR, we now have a minimum supported version listed for all of our dependencies, and we actively test to ensure that this stays up-to-date with each commit.

In the past, I tried not to overly restrict our dependencies if I didn't know the oldest supported version. With this PR, I'm taking the policy of "the oldest supported version is the oldest we can test". For example, many of our dependencies are pinned to versions due to Python 3.7 support or because another dependency requires a newer version, even if an older version would work with our code.

Closes #32

setup.cfg

adamjstewart · 2022-06-13T04:18:25Z

setup.cfg

+    # matplotlib 3.3+ required for (H, W, 1) image support in plt.imshow
+    matplotlib>=3.3,<4


Some of our datasets (idtrees?) are plotting (H, W, 1) images, can we just squash these so we can support older matplotlib?

Should we keep track of these types of questions somewhere? Or resolve them here?

I'm fine with either. If I get bored I'll try to fix these and support older versions. If I get busy I'll just merge this as is and deal with it another day.

adamjstewart · 2022-06-13T04:20:11Z

setup.cfg

-    rasterio>=1.0.16,<2
+    # pytorch-lightning 1.5.1+ required for apply_to_collection bugfix
+    pytorch-lightning>=1.5.1,<2
+    # rasterio 1.0.20+ required for out_dtype parameter of DatasetReaderBase.read


Unclear if this is really needed, could just cast to different dtype after reading

calebrob6 · 2022-06-13T16:45:49Z

Kk I'm happy with this regardless

adamjstewart · 2022-06-13T17:37:30Z

TODO: update environment.yml

pyproject.toml

adamjstewart · 2022-06-15T06:47:06Z

environment.yml

  - open3d>=0.11.2
  - pip
-  - protobuf<4


No need to pin this here, it isn't used by CI so it won't cause us issues. If someone encounters an issue they can manually install an older protobuf.

adamjstewart · 2022-06-15T06:47:44Z

setup.cfg

-    # Required to address protocolbuffers/protobuf#10051
-    protobuf<4


No need to pin this here, it isn't used by CI so it won't cause us issues. If someone encounters an issue they can manually install an older protobuf.

adamjstewart · 2022-06-15T06:48:18Z

requirements/requirements.txt

@@ -47,7 +47,6 @@ scipy==1.7.3;python_version=='3.7'
 zipfile-deflate64==0.2.0

 # docs
-ipywidgets==7.7.0


I added this once upon a time because I ran into issues locally, but the doc tests seem to pass without it and it isn't a direct dep, so I removed it

adamjstewart · 2022-06-15T06:50:00Z

torchgeo/datasets/advance.py

@@ -199,7 +199,7 @@ def _load_target(self, path: str) -> Tensor:
                "scipy is not installed and is required to use this dataset"
            )

-        array = wavfile.read(path)[1]
+        array = wavfile.read(path, mmap=True)[1]


Without this I was seeing an issue with older scipy/torch:

tests/datasets/test_advance.py::TestADVANCE::test_getitem /home/adam/torchgeo/torchgeo/datasets/advance.py:203: UserWarning: The given NumPy array is not writeable, and PyTorch does not support non-writeable tensors. This means you can write to the underlying (supposedly non-writeable) NumPy array using the tensor. You may want to copy the array to protect its data or make it writeable before converting it to a tensor. This type of warning will be suppressed for the rest of this program. (Triggered internally at /pytorch/torch/csrc/utils/tensor_numpy.cpp:180.) tensor = torch.from_numpy(array)

It doesn't seem to be needed for newer scipy/torch, but as long as the tests pass I don't see any harm in always using this.

adamjstewart · 2022-06-15T06:50:17Z

torchgeo/datasets/cyclone.py

-                except AttributeError:  # pragma: no cover
+                except AttributeError:


We can finally properly test these lines!

Finallllly!!!!!!

adamjstewart · 2022-06-15T06:51:34Z

pyproject.toml

    # https://github.com/rwightman/pytorch-image-models/pull/1256
-    "ignore:.*is deprecated and will be removed in Pillow 10:DeprecationWarning",


If we don't specify a module, this will ignore the test for all files, including our own. By specifying a model, we can ignore things with more fine-grained precision. We should probably do this for all warnings raised by deps of deps.

adamjstewart · 2022-06-15T06:52:36Z

pyproject.toml

+    "ignore:Named tensors and all their associated APIs are an experimental feature and subject to change.:UserWarning:torch.nn.functional",
+    # https://github.com/numpy/numpy/issues/14920
+    # https://github.com/numpy/numpy/issues/15748
+    "ignore:numpy.ufunc size changed, may indicate binary incompatibility:RuntimeWarning",


Only occurs for the minimum version tests, but basically, at least one of our deps was built with a different version of numpy than we are using to run the tests. Doesn't seem to cause any harm other than the warning.

adamjstewart · 2022-06-15T07:08:56Z

@calebrob6 this should be ready to merge now assuming the tests pass!

calebrob6 · 2022-06-15T07:10:07Z

Sick, nice work!!

calebrob6 · 2022-06-15T07:23:40Z

Just to make sure I'm understanding at the high level:

requirements/requirements.txt is now managed by dependabot, who bumps our pinned versions every time some upstream package has a release. This way, if something breaks, then we know in the dependabot PR, instead of all of our other CI breaking all of the sudden.
requirements/requirements-min.txt is now managed by us. This is just nice to have? We update this whenever we add a new dependency (which we don't like doing), when we drop a dependency, or when we do something like drop support for a Python version.
We're now explicitely ignoring warnings that are generated by our dependencies (that we don't have control over), mainly to stop cluttering up the tests. We're responsible for checking to see if these have changed every once in a while, but its not a huge deal.

Is this correct?

adamjstewart added the testing Continuous integration testing label Jun 12, 2022

github-actions bot removed the testing Continuous integration testing label Jun 12, 2022

adamjstewart added the testing Continuous integration testing label Jun 13, 2022

github-actions bot removed the testing Continuous integration testing label Jun 13, 2022

adamjstewart added this to the 0.3.0 milestone Jun 13, 2022

adamjstewart commented Jun 13, 2022

View reviewed changes

adamjstewart marked this pull request as ready for review June 13, 2022 04:21

calebrob6 previously approved these changes Jun 13, 2022

View reviewed changes

adamjstewart marked this pull request as draft June 13, 2022 17:37

adamjstewart dismissed calebrob6’s stale review via 46f2759 June 13, 2022 17:48

adamjstewart force-pushed the tests/min branch from df2246e to 46f2759 Compare June 13, 2022 17:48

github-actions bot added the testing Continuous integration testing label Jun 13, 2022

adamjstewart marked this pull request as ready for review June 13, 2022 17:55

adamjstewart force-pushed the tests/min branch from eef15e8 to 7cb28ee Compare June 13, 2022 23:07

adamjstewart marked this pull request as draft June 13, 2022 23:08

adamjstewart commented Jun 15, 2022

View reviewed changes

pyproject.toml Show resolved Hide resolved

github-actions bot added the datasets Geospatial or benchmark datasets label Jun 15, 2022

adamjstewart commented Jun 15, 2022

View reviewed changes

adamjstewart marked this pull request as ready for review June 15, 2022 07:08

adamjstewart added 21 commits June 16, 2022 17:52

bump sklearn

c8b6461

wheel no longer needed in tests

892e876

bump pandas

c9a2afd

bump pillow

e1181a4

bump numpy

729c943

undo

0783fb9

uninstall sklearn

1972a30

yes

6465468

see if removing open3d helps

700c5fe

Try newer sklearn

8a82aa8

Try bumping scipy

594f24e

older sklearn

de12605

sklearn

eefb3fc

sklearn

3000ecd

sklearn

e25f186

sklearn

6fef3a8

bump scipy

9385cf1

scipy

533e813

remove scipy hacks

5301121

Restore scipy hacks

80c7d67

Bump test dep versions

b90a6cb

adamjstewart dismissed calebrob6’s stale review via b90a6cb June 17, 2022 01:12

adamjstewart force-pushed the tests/min branch from 28e9fc9 to b90a6cb Compare June 17, 2022 01:12

adamjstewart marked this pull request as ready for review June 17, 2022 01:13

calebrob6 approved these changes Jun 18, 2022

View reviewed changes

adamjstewart merged commit 8c5e4d2 into main Jun 18, 2022

adamjstewart deleted the tests/min branch June 18, 2022 15:00

adamjstewart mentioned this pull request Jun 18, 2022

Move requirements-min.txt, split deps in multiple files #605

Merged

2 tasks

adamjstewart mentioned this pull request Jul 11, 2022

0.3.0 release #664

Merged

yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023

Test minimum version of deps (microsoft#574)

cf04f4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test minimum version of deps #574

Test minimum version of deps #574

adamjstewart commented Jun 12, 2022 •

edited

Loading

adamjstewart Jun 13, 2022

calebrob6 Jun 13, 2022

adamjstewart Jun 13, 2022

adamjstewart Jun 13, 2022

calebrob6 commented Jun 13, 2022

adamjstewart commented Jun 13, 2022

adamjstewart Jun 15, 2022

adamjstewart Jun 15, 2022

adamjstewart Jun 15, 2022

adamjstewart Jun 15, 2022

adamjstewart Jun 15, 2022

calebrob6 Jun 15, 2022

adamjstewart Jun 15, 2022

adamjstewart Jun 15, 2022

adamjstewart commented Jun 15, 2022

calebrob6 commented Jun 15, 2022

calebrob6 commented Jun 15, 2022

		# matplotlib 3.3+ required for (H, W, 1) image support in plt.imshow
		matplotlib>=3.3,<4

		# Required to address protocolbuffers/protobuf#10051
		protobuf<4

		except AttributeError: # pragma: no cover
		except AttributeError:

		# https://github.com/rwightman/pytorch-image-models/pull/1256
		"ignore:.*is deprecated and will be removed in Pillow 10:DeprecationWarning",

Test minimum version of deps #574

Test minimum version of deps #574

Conversation

adamjstewart commented Jun 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebrob6 commented Jun 13, 2022

adamjstewart commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamjstewart commented Jun 15, 2022

calebrob6 commented Jun 15, 2022

calebrob6 commented Jun 15, 2022

adamjstewart commented Jun 12, 2022 •

edited

Loading