Skip to content
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

Resolve DeprecationWarnings #567

Merged
merged 11 commits into from
Jun 13, 2022
Merged

Resolve DeprecationWarnings #567

merged 11 commits into from
Jun 13, 2022

Conversation

adamjstewart
Copy link
Collaborator

@adamjstewart adamjstewart commented Jun 8, 2022

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

If you look at the logs for our unit tests, you'll see a slew of warning messages, desperately trying to warn us that TorchGeo is using deprecated features that will be removed in future versions of our dependencies. Up until now, these were ignored until someone noticed/reported an issue. With this PR, these kind of warnings will become fatal. We weren't able to do something this extreme before, but now that we use dependabot, we can restrict these kind of failures to version update PRs, not user PRs.

Deprecation Fixes

  • Rtree 1.0.0 introduced a new len(index) method to replace index.get_size() (see Add __len__ method to Index Toblerity/rtree#207). The latter is now deprecated and may be removed someday.
  • Pillow 9.1.0 relocated all resampling methods from Image.* to Image.Resampling.*. The former is now deprecated and will be removed in Pillow 10

Considerations

  • When a dependency changes its API, should we use the new API and bump the required version of that dependency, or should we maintain compatibility with historic versions?

I did the former for rtree/torchmetrics/other instances because I didn't see a good reason not to, but I did the latter for pillow because pillow-simd doesn't yet have a 9.1.0 release and normal pillow is much slower. There's also the question of how to maintain coverage of both branches. This may require a test using the minimum supported versions: #32.

  • What's the best way to handle warnings in our dependencies, not in our own code?

So far I'm just ignoring these using filterwarnings, but that may not be sufficient. What I would really love is an option to only issue warnings for our own code, not for dependencies, but I don't believe that's an option. One potential issue here is if a dep of a dep has a new release that causes the tests to fail, but it isn't managed by dependabot because it's not a direct dep.

  • How long should we keep around warning ignores? Even after they no longer exist in newer versions?

Ideally, our tests will pass not only on CI which uses the latest versions of everything, but also locally where users may have any version of these dependencies installed. I'm not sure if it's worth keeping a running list of all possible warning messages for all versions of our dependencies. One option would be to remove "error" from filterwarnings in pyproject.toml and instead use pytest -Werror in CI so that warnings are only considered errors in CI, not locally.

@github-actions github-actions bot added the datasets Geospatial or benchmark datasets label Jun 8, 2022
@adamjstewart adamjstewart added this to the 0.2.2 milestone Jun 9, 2022
@github-actions github-actions bot added the testing Continuous integration testing label Jun 9, 2022
@github-actions github-actions bot added the datamodules PyTorch Lightning datamodules label Jun 9, 2022
@calebrob6
Copy link
Member

re Pillow being really slow, we do already have an opencv dependency, so could use that

@adamjstewart
Copy link
Collaborator Author

According to pytorch/vision#6153, Pillow-SIMD is actually faster than OpenCV and Tensor operations on the GPU.

@adamjstewart adamjstewart merged commit 7a9a9c1 into main Jun 13, 2022
@adamjstewart adamjstewart deleted the warnings branch June 13, 2022 17:38
@adamjstewart adamjstewart modified the milestones: 0.2.2, 0.3.0 Jul 2, 2022
@adamjstewart adamjstewart mentioned this pull request Jul 11, 2022
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Resolve DeprecationWarnings

* Fix PIL version check

* Add ignores, hide ignores

* Always specify max_epochs

* Re-add one of the ignores that was removed

* Silence ColorJitter behavior change warnings

* Fix num_workers warning on macOS

* Ignore coverage of lines missed by older pillow versions

* Fix support for pre/post versions of PIL

* Wordsmithing

* Remove unused imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datamodules PyTorch Lightning datamodules datasets Geospatial or benchmark datasets testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants