Fix circular imports and introduce isort

[duckontheweb]

Related Issue(s):

• Introduce isort #533
• feat: Introduce isort #534 (previous PR addressing this issue)
• refactor: Avoid implicit reexport in submodules #539 (discussion on import statement style)

Description:

• Introduces isort for sorting imports
• Uses a consistent import style throughout the code that follows the Google Python Style Guide
• Uses TYPE_CHECKING gate for imports that are only used in type checking
• Moves Item, Catalog, and Collection classes into core module to avoid circular imports
• Introduces bump2version for managing package version changes instead of reading from package dynamically.

I do not have a strong opinion on sorting of imports or the style of import that we use throughout the code, but introducing these changes uncovered and fixed a number of circular import issues. Those circular imports have worked up until now only by keeping a certain order to the imports in the pystac/__init__.py. This has caused problems on a few occasions where I have added or moved modules and had to figure out where to insert them into the pystac/__init__.py import order by trial and error. Introducing isort doesn't solve this problem, but it does make sure we don't paper over it with a specific import order in that __init__ file.

Using the Google style of imports does seem to help with some of the circular imports since it avoids import partially initialized modules in most case. I also moved the Item, Catalog, and Collection code into a single core module because there were legitimate circular imports between those modules that could not be resolved by the TYPE_CHECKING gating.

This PR could represent a breaking change to anyone who has been importing things directly from pystac.item, pystac.collection, or pystac.catalog instead of from the the top-level package. If we think this is a big enough concern I can re-introduce those modules and explicitly re-export the classes with a deprecation warning for now, but given that all of our docs and examples demonstrate importing these classes from the top-level module, it didn't seem worth that extra complexity. (UPDATE: This change was reverted based on feedback on this PR).

UPDATE: This also introduces bump2version for managing changes to this package's version. Previously setup.cfg was reading this dynamically from pystac.version.__version__, but since pystac and its dependencies are not necessarily installed by the time setuptools read this information this could cause problems. Using bump2version allows us to manage this as a static string, but still update it everywhere it needs updating when we change versions.

PR Checklist:

• Code is formatted (run pre-commit run --all-files)
• Tests pass (run scripts/test)
• Documentation has been updated to reflect changes, if applicable
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

cc: @l0b0

[codecov-commenter]

Codecov Report

Merging #594 (f93cff3) into main (cafa5ab) will decrease coverage by 0.04%.
The diff coverage is 93.16%.

@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
- Coverage   94.69%   94.64%   -0.05%     
==========================================
  Files          75       73       -2     
  Lines       10776    10725      -51     
  Branches     1053     1053              
==========================================
- Hits        10204    10151      -53     
- Misses        396      397       +1     
- Partials      176      177       +1     

Impacted Files	Coverage Δ
tests/extensions/test_datacube.py	100.00% <ø> (ø)
tests/extensions/test_item_assets.py	100.00% <ø> (ø)
pystac/common_metadata.py	92.59% <50.00%> (-0.14%)	⬇️
pystac/extensions/datacube.py	63.39% <72.72%> (-0.89%)	⬇️
pystac/cache.py	92.30% <84.61%> (ø)
pystac/serialization/common_properties.py	78.84% <87.50%> (-0.79%)	⬇️
pystac/extensions/label.py	93.67% <88.57%> (+0.01%)	⬆️
pystac/extensions/raster.py	89.70% <89.47%> (ø)
pystac/stac_object.py	90.95% <90.00%> (+0.09%)	⬆️
pystac/link.py	90.53% <91.30%> (+0.22%)	⬆️
... and 58 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafa5ab...f93cff3. Read the comment docs.

[gadomski]

I'm broadly 👍🏽 on this change. I don't like having to think about import order, so this takes care of that for me; I'm also 👍🏽 on namespacing free functions, etc. Future-proofing against import hell is also a good thing. The big core.py file isn't ideal but sometimes that's what has to happen.

[gadomski]

This I think is for sure a breaking change, and if there's a way to keep these imports working (with a deprecation notice) I'd be in favor. I find, for example, that some IDE's auto-import from the deeper path, so folks could be relying on the original import locations without even knowing it.

[duckontheweb]

I got things back to using separate item, catalog, and collection modules in the latest push. I was able to resolve most of the import issues by being more careful about using the TYPE_CHECKING gate on imports that were only used for type checking, and by using the new module import style.

There was 1 circular dependency that couldn't be cleanly resolved using these strategies, which was between any of item, catalog, and collection and stac_io. The root cause is that each of those modules uses stac_io to call stac_io.StacIO.default() in some methods that have an optional stac_io argument, and the stac_io module itself depends on item, catalog, and collection as part of the logic in stac_object_from_dict. I think there's probably a more elegant solution to be had here, but to work around this, I simply added a default_stac_io method to pystac/__init__.py that those other modules can use to break the circular dependency.

[duckontheweb]

The big core.py file isn't ideal but sometimes that's what has to happen.

Yeah, this was by far my least favorite part of this change. I'll take another pass at making the imports work with the old item, collection, and catalog modules given the feedback on it being a breaking change as well.

[lossyrob]

The new import style feels very verbose and foreign. There's a lot of aliasing going on (e.g. from pystac.serialization import migrate as migrate_mod). Is there some tooling to enforce this type of import structure, and to recognize when aliases are needed, or is this ad-hoc/based on understanding and adherence to some guidelines? If the latter, then it seems like this will be very difficult to maintain as a practice given the number of contributors we'd like to target, without reviewers being very vigilant about the import statements of contributions. I forsee most contributors not wanting (or knowing how or why) to type things like

from pystac import stac_object as stac_object_mod
x = stac_object_mod.STACObjectType.COLLECTION

and there will be a lot of PR churn on getting import statements correct in second or third passes.

I understand the desire to reduce the import burden in __init__.py, though this seems like it should only effect deeper changes like restructuring or new types, and not a majority of contributions. Perhaps the cost I see of adding these changes is worth it; however it does feel a bit like we're externalizing the complexity of the init.py imports to developers that wouldn't normally have to think about it. This seems to conflict with the idea that these changes should let contributors not think about imports, which perhaps is due to my own lack of understanding of how simple it is to find and bring types out of mid-level subpackages without having to do a second pass to correct imports according to the style.

That said, and as I mentioned in the last isort PR, if it seems like generally contributors are positive to this change, I'm willing to go along with it.

[duckontheweb]

The new import style feels very verbose and foreign.

I agree, this seems a bit awkward, and we end up having to use it in a number of places due to naming conflicts.

Is there some tooling to enforce this type of import structure, and to recognize when aliases are needed, or is this ad-hoc/based on understanding and adherence to some guidelines? If the latter, then it seems like this will be very difficult to maintain as a practice given the number of contributors we'd like to target, without reviewers being very vigilant about the import statements of contributions.

I don't know of any tooling that would enforce this, and I agree this puts a lot of pressure on both developers and reviewers to get this right.

however it does feel a bit like we're externalizing the complexity of the __init__.py imports to developers that wouldn't normally have to think about it. This seems to conflict with the idea that these changes should let contributors not think about imports

I agree here as well. When I embarked on this I was hoping I would be able to simplify the way our imports worked, but I think in the end we're just trading one kind of pain for another.

I'll leave this PR open for a bit in case other contributors and maintainers have other opinions, but having gone through the work now of fleshing this out I'm not sure the change is worth it either. Maybe someone else can come up with a better approach for resolving the circular import issues that doesn't also introduce some of the problems here.