CI: test optional datasets on every commit #2045
Conversation
| @@ -14,7 +14,6 @@ | |||
| class TestUSAVarsDataModule: | |||
| @pytest.fixture | |||
| def datamodule(self, request: SubRequest) -> USAVarsDataModule: | |||
| pytest.importorskip('pandas', minversion='1.1.3') | |||
There was a problem hiding this comment.
Unrelated to this PR, but this dep is now required
| @@ -89,7 +89,7 @@ def test_trainer( | |||
| self, monkeypatch: MonkeyPatch, name: str, fast_dev_run: bool | |||
| ) -> None: | |||
| if name.startswith('so2sat') or name == 'quakeset': | |||
| pytest.importorskip('h5py', minversion='3') | |||
| pytest.importorskip('h5py', minversion='3.6') | |||
There was a problem hiding this comment.
We forgot to update the minimum deps when dropping Python 3.9 support
| monkeypatch.setattr(LandCoverAI, 'sha256', sha256) | ||
| match name: | ||
| case 'chabud': | ||
| pytest.importorskip('h5py', minversion='3.6') |
There was a problem hiding this comment.
This was missing and caused the release branch tests to fail without this fix
| """ | ||
| try: |
There was a problem hiding this comment.
No need to fail immediately upon instantiation, h5py is only needed for __getitem__
There was a problem hiding this comment.
I disagree, without h5py instantiation won't be useful? I'd rather find out immediately that my dataset won't work.
There was a problem hiding this comment.
It does allow you to download/extract the dataset and plot images, just not load images. I get your point though. How long of a delay is there in realizing that the dataset doesn't work if we don't error until getitem?
| @@ -182,13 +181,6 @@ def __init__( | |||
| self.num_classes = len(self.classes) | |||
| self._verify() | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
It wasn't easy to test missing pyvista when laspy is also missing without this change.
There was a problem hiding this comment.
Same here -- I think the dataset should fail to instantiate if the deps aren't installed
There was a problem hiding this comment.
I think I have an idea that will allow us to fail-fast and still get full coverage, will open another prereq PR with just that change so that this PR can be a test-only PR.
| @@ -27,6 +27,7 @@ class TestQuakeSet: | |||
| def dataset( | |||
| self, monkeypatch: MonkeyPatch, tmp_path: Path, request: SubRequest | |||
| ) -> QuakeSet: | |||
| pytest.importorskip('h5py', minversion='3.6') | |||
There was a problem hiding this comment.
This was missing and caused the tests to fail on the release branch.
| @@ -127,10 +116,6 @@ def download_url(url: str, root: str, *args: str) -> None: | |||
| shutil.copy(url, root) | |||
|
|
|||
|
|
|||
| def test_mock_missing_module(mock_missing_module: None) -> None: | |||
There was a problem hiding this comment.
Unit tests for our unit tests. I love it.
TorchGeo has a number of optional dependencies only needed by certain datasets. Until now, we've only ensured that all tests pass without these dependencies installed on release branches. However, this makes it difficult to get full coverage without relying on ugly import monkeypatching. It's simpler and easier to test both on every commit to get full coverage. We already do this for the minimum version of our dependencies.
Prerequisite for #2043