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

cannot import name 'RandomBatchGeoSampler' from 'torchgeo.samplers.batch' #276

Closed
RitwikGupta opened this issue Dec 11, 2021 · 5 comments
Closed
Milestone

Comments

@RitwikGupta
Copy link
Collaborator

On torchgeo==0.1.0, the import from torchgeo.samplers import GeoSampler fails with an ImportError. Interestingly, re-running the same import immediately after succeeds.

>>> from torchgeo.samplers import GeoSampler
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/shared/ritwik/miniconda3/envs/overwatch/lib/python3.7/site-packages/torchgeo/samplers/__init__.py", line 6, in <module>
    from .batch import BatchGeoSampler, RandomBatchGeoSampler
  File "/shared/ritwik/miniconda3/envs/overwatch/lib/python3.7/site-packages/torchgeo/samplers/batch.py", line 12, in <module>
    from torchgeo.datasets.geo import GeoDataset
  File "/shared/ritwik/miniconda3/envs/overwatch/lib/python3.7/site-packages/torchgeo/datasets/__init__.py", line 11, in <module>
    from .chesapeake import (
  File "/shared/ritwik/miniconda3/envs/overwatch/lib/python3.7/site-packages/torchgeo/datasets/chesapeake.py", line 26, in <module>
    from ..samplers.batch import RandomBatchGeoSampler
ImportError: cannot import name 'RandomBatchGeoSampler' from 'torchgeo.samplers.batch' (/shared/ritwik/miniconda3/envs/overwatch/lib/python3.7/site-packages/torchgeo/samplers/batch.py)
>>> GeoSampler
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'GeoSampler' is not defined
>>> from torchgeo.samplers import GeoSampler
>>> GeoSampler
<class 'torchgeo.samplers.GeoSampler'>
>>> 
@RitwikGupta
Copy link
Collaborator Author

I cannot reproduce this issue on torchgeo==0.2.0.dev0. I'll leave this open since 0.1.0 is the version on PyPi and conda-forge.

@ashnair1
Copy link
Collaborator

ashnair1 commented Dec 11, 2021

This is due to a circular dependency. Importing GeoSampler requires importing GeoDataset which in turn imports all datasets (including chesapeake.py which required RadomBatchGeoSampler). This was resolved post 0.1.0 by b67c711 which imports the datasets in torchgeo's __init__.py to ensure it's imported first.

In your case, I guess it worked the second time because by then the datasets submodule was already imported by the previous failed call.

@adamjstewart adamjstewart added this to the 0.1.1 milestone Dec 11, 2021
@adamjstewart
Copy link
Collaborator

@RitwikGupta thanks for the bug report, and @ashnair1 thanks for figuring out which commit fixed this. We'll likely release a 0.1.1 or 0.2.0 release in the next week that should resolve this.

@adamjstewart
Copy link
Collaborator

I'm actually thinking about removing the imports in torchgeo/__init__.py due to the resulting overhead it causes (see #287 (comment)). However, this gets us back to the issue of circular imports. To recap:

  • Our sampler code needs to import from torchgeo.datasets for type annotations (GeoDataset) and for run-time usage (BoundingBox)
  • Our datamodule code needs to import from torchgeo.samplers for run-time usage (GeoSampler)

This issue arose when we decided to move all of our datamodules to torchgeo.datasets. There are a few ways to prevent this circular import:

  1. Add import torchgeo.datasets in torchgeo/__init__.py (current solution, adds overhead)
  2. Move BoundingBox to a separate torchgeo.utils directory or use lazy imports, remove or guard type hints (hacky, may have same issue again in the future)
  3. Move datamodules to their own directory torchgeo.datamodules or back to torchgeo.trainers (disruptive, not backwards compatible)

I'm fine with being disruptive (TorchGeo is still in alpha, we can afford to make backwards incompatible changes), but I want to make sure we have a good design first. Our goal here is to organize the library such that each directory within torchgeo is as decoupled as possible.

@adamjstewart
Copy link
Collaborator

The original issue reported in this issue (circular import errors) has been fixed in the new 0.1.1 release, so I'll close this PR.

I do still want to think about ways to speed up imports, but that can happen later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants