Refactors architecture to better enable STAC 1.0 extension changes

[lossyrob]

Related Issue(s): #306

Description:

This PR will contain work to refactor some core architectural pieces in preparation for the STAC 1.0 release. One of the most disruptive changes is that Extensions were moved off of a distinction between "core" and custom extensions, and the usage of a short ID in the stac_extensions field has been removed in favor of identifying extensions only by their JSON schema URL. PySTAC use these IDs as a core component of how it handled extensions, and so this will need to change significantly moving forward. After discussions with some other PySTAC developers and users, it also became clear that the current extension implementation is complex and clunky. I believe now's a good time to rethink how we're doing extensions, and performing a few core refactors of PySTAC prior to the 1.0 release that should make things more simple and better suited to the most recent version of the spec.

I'm putting up a WIP PR early to let folks know what's happening and elicit any feedback as this work occurs. My plan is to approach the work like this:

• Add type annotations to everything. This will cause breakages due to circular imports, but will make refactoring easier and will likely expose some type organization issues that will be good to clean up (and will solve Add type annotations to PySTAC #260)
• Attempt to resolve circular imports. This may take a bit of refactoring, and it may be useful to do the extensions rework while this happens, but ideally I can get to a point where tests pass again with full annotations.
• Rework the extensions
• Fix tests
• Implement IO changes
• Reformat codebase using black (Move to using black as the formatter #316)

Left TODO -

• Update CHANGELOG

Docstring/Documentation updates, as well as bringing test coverage back up to 93% and some additional implementations/fixes for Collection/Catalog summaries and extensions are encapsulated in issues and left to follow up work.

Extensions have a new API, e.g.

item: pystac.Item = ...
item.ext.enable("eo")
item.ext.eo.cloud_cover = 0.7
print(item.ext.projection.bbox)

is now

from pystac.extensions.eo import EOExtension

item = ...

EOExtension.add_to(item)
eo_ext = EOExtension.ext(item)
eo_ext.cloud_cover = 0.7
print(eo_ext.cloud)

PR Checklist:

• Code is formatted (run scripts/format)
• Tests pass (run scripts/test)
• 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.

Fixes #293
Fixes #308
Fixes #311
Fixes #313
Fixes #316
Fixes #266
Fixes #260
Fixes #242
Fixes #205
Fixes #132
Fixes #30

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@b005962). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #309   +/-   ##
=======================================
  Coverage        ?   88.60%           
=======================================
  Files           ?       36           
  Lines           ?     4625           
  Branches        ?        0           
=======================================
  Hits            ?     4098           
  Misses          ?      527           
  Partials        ?        0           

---

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 b005962...63f83f4. Read the comment docs.

[gadomski]

The new stac_io stuff makes sense and looks good to me. I did my best to go through each file's functional changes but it was hard to differentiate formatting/typing changes from the real deal; no obvious WTFs so 👍🏽 .

I will make one comment that's more of a STAC issue than a PySTAC issue, but I still don't love when I see, e.g., get_children returning Catalogs. I understand (I think) why Collection does not inherit from Catalog in the spec but it does here in pystac, and I don't think it matters for the vast majority of users, but it still scans funny during a review. Don't know what the answer is though (without adding like a Container supertype that encompasses both Catalogs and Collectionss and then we're just spamming types).

[lossyrob]

@gadomski I believe we can solve that issue via types by using

def get_children(self): Iterable[Union[Catalog, Collection]]:
   ...

which doesn't mean a lot for the type checker since Collection inherits from Catalog, but would be more clear to a user. Would that resolve the issue you brought up around the get_children confusion? If so I'll make that change to all of the child methods on Catalog.

[gadomski]

Would that resolve the issue you brought up around the get_children confusion?

In that case, yes -- I think there were some other cases where you did a cast(Catalog) in the body of a function, and my static-typed brain thought we were losing information when in reality you're just making the type checker happy. It's not a huge deal, but yes I do think at least in function signatures being explicit about returning a Union[Catalog, Collection] aligns better with the spec (IMO).

Comments were resolved

[matthewhanson]

This all looks good to me, although I primarily looked through the STAC_IO changes.

I'm not sure I understand the value in deprecating the STAC_IO class though. None of those functions will ever be called by the new code, and if someone overrides a function like before they won't get an deprecation warning or anything.

Is there value in keeping the code around? Also, I'm not found of the file being stac_io.py while the new class is StacIO, which breaks the naming style. I think it would be fine to completely replace the old STAC_IO class with the new one.

I might have some more feedback when updating pystac-client, but see no reason to not merge this, and I'll get pystac-client working with the main branch in the next few days.

[lossyrob]

I'm not sure I understand the value in deprecating the STAC_IO class though. None of those functions will ever be called by the new code, and if someone overrides a function like before they won't get an deprecation warning or anything.

I've definitely used STAC_IO in various other projects as a convenient way to piggyback on PySTAC functionality; I think there is usage of it directly in stactools. The deprecation allows anyone who was doing that to avoid breaking their codebases while upgrading.

Is there value in keeping the code around? Also, I'm not found of the file being stac_io.py while the new class is StacIO, which breaks the naming style. I think it would be fine to completely replace the old STAC_IO class with the new one.

I think this does follow the convention? e.g. MediaType and media_type.py. I'd consider the "io" as a separate word so the underscore would separate them in the file name.

Let me know what you think on the above two points; if it makes sense to merge as-is I'll do that once you give a final +1

[matthewhanson]

@lossyrob
Ah-ha, I see it may be used downstream. Ok, sounds good then.

Also, you are right on the convention, in fact it didn't follow the standard Python convention for Class naming using CapWords before, so carry on. +1!

[lossyrob]

SoundsGoodToMe :-)