Rework list of required dependencies #287
Conversation
| - name: Run mypy checks | ||
| run: mypy . | ||
| datasets: |
There was a problem hiding this comment.
Identical to our other tests but without the optional dataset dependencies to ensure that we are skipping them properly.
| @@ -37,6 +37,8 @@ install_requires = | |||
| kornia>=0.5.4 | |||
| matplotlib | |||
| numpy | |||
| # omegaconf 2.1+ required for to_object method | |||
| omegaconf>=2.1 | |||
There was a problem hiding this comment.
Technically omegaconf isn't used in torchgeo proper, only in train.py/benchmark.py and the unit tests, but it seemed weird to keep a train list of optional deps with only one dep...
| @@ -57,6 +56,7 @@ def mocked_import(name: str, *args: Any, **kwargs: Any) -> Any: | |||
| ) | |||
|
|
|||
| def test_getitem(self, dataset: ADVANCE) -> None: | |||
| pytest.importorskip("scipy", minversion="0.9.0") | |||
There was a problem hiding this comment.
Scipy is only needed for __getitem__, it isn't needed for any other part of the dataset.
| @@ -19,6 +19,9 @@ | |||
| import torchgeo.datasets.utils | |||
| from torchgeo.datasets import IDTReeS | |||
|
|
|||
| pytest.importorskip("pandas", minversion="0.19.1") | |||
| pytest.importorskip("laspy", minversion="2") | |||
There was a problem hiding this comment.
Some of the tests that don't use the dataset fixture still use IDTReeS and will fail if these deps aren't installed.
|
My only other complaint about our imports is that things are currently very slow. Because $ time python -c 'from torchgeo.datasets import VHR10'
real 0m8.428s
user 0m5.748s
sys 0m1.856sIf we didn't use import aliases, this would be significantly faster. It's only really an issue when you want to test stuff on the command-line since scripts will take a long time to run regardless. |
|
I think right now most users are here for the datasets/samplers. Could we not import the trainers/models automatically? I'm wondering how much of a difference just not importing those makes. |
Currently, because we import datasets/trainers in $ time python -c 'from torchgeo.datasets.nwpu import VHR10'
real 0m7.549s
user 0m5.553s
sys 0m1.784sIf we remove all imports in $ time python -c 'from torchgeo.datasets.nwpu import VHR10'
real 0m5.452s
user 0m3.975s
sys 0m1.451sIf we also remove all imports in $ time python -c 'from torchgeo.datasets.nwpu import VHR10'
real 0m3.302s
user 0m2.377s
sys 0m0.909s |
|
The imports in |
* Rework list of required dependencies * Update open3d import error msg * Style fixes * Remove extra empty line * Increase test coverage * Fix idtrees tests
* Rework list of required dependencies * Update open3d import error msg * Style fixes * Remove extra empty line * Increase test coverage * Fix idtrees tests
With this PR, I would like to introduce a new definition of required dependencies: anything that is required to import any part of the library. Any optional dependencies, such as those listed under the
[options.extras_require]section ofsetup.cfgmust be guarded by try-excepts to notify the user that a certain library is missing and is needed for the feature they are trying to use. Many package managers, including Spack and Conda, will try to import sections of the library to ensure that things were installed correctly. These tests shouldn't fail because optional dependencies are missing. To enforce this, I've added a test that doesn't install the optional dataset dependencies to ensure that the tests still pass.Closes #72
Related to #230, #249