Fix STACObject inheritance #451

duckontheweb · 2021-06-17T15:00:05Z

Related Issue(s): #

Casting root Catalog to Catalog type interferes with pystac-client Client class #410

Description:

Updates from_dict and from_file methods for STACObject and all sub-classes so that any classes inheriting from them will be returned by those methods. This makes it easier for downstream libraries like pystac-client to create custom sub-classes. See these tests for an example.

Also moves the logic from serialization.stac_object_from_dict into stac_io to simplify the chain of functions that is called. If it seems like downstream libraries are using this function directly we can go through a deprecation process, but it seemed unlikely. Once STAC_IO is removed in 1.0.0 this code will only be used in one place in the StacIO class, so it seemed to be simpler to just move it in there.

Supersedes #443

PR Checklist:

Code is formatted (run pre-commit run --all-files)
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.

duckontheweb · 2021-06-17T15:08:01Z

@matthewhanson I believe this will close #410, but would like to get your confirmation that it covers the needs in that issue.

lossyrob · 2021-06-17T15:36:02Z

@duckontheweb Want to clarify before review - can you talk about why STACObject.from_file uses logic that seems to be duplicated in StacIO.read_stac_object, when it was just a straight call before? It seems like this introduces that logic in a couple of different places (e.g. also pystac.read_file), where it could be consolidated into the StacIO method. I agree with the move out of serialization into StacIO, just not sure why the identify, migrate etc checks are duplicated in other places.

duckontheweb · 2021-06-17T15:54:38Z

@duckontheweb Want to clarify before review - can you talk about why STACObject.from_file uses logic that seems to be duplicated in StacIO.read_stac_object, when it was just a straight call before? It seems like this introduces that logic in a couple of different places (e.g. also pystac.read_file), where it could be consolidated into the StacIO method. I agree with the move out of serialization into StacIO, just not sure why the identify, migrate etc checks are duplicated in other places.

Yeah, definitely a good question. StacIO.read_stac_object calls StacIO.stac_object_from_dict, which has hard-coded classes (pystac.Catalog, pystac.Collection, pystac.Item) that it calls from_dict methods on to get a STACObject sub-class instance. For custom subclasses (e.g. class API(pystac.Catalog)) this breaks the ability to get back the original calling class in from_file.

I originally considered passing the cls argument from that method down through the chain, but it seemed like it was going to change the signatures of too many functions, and I didn't want to introduce that change.

I agree the duplication of logic is not ideal, and makes maintenance trickier. I can take another look at this and see if there is a better solution that doesn't involve altering the StacIO method signatures and gives us some DRY-er code.

codecov-commenter · 2021-06-17T19:15:28Z

Codecov Report

Merging #451 (b73ac4d) into main (a729cd2) will increase coverage by 0.16%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   91.31%   91.47%   +0.16%     
==========================================
  Files          40       40              
  Lines        5204     5234      +30     
==========================================
+ Hits         4752     4788      +36     
+ Misses        452      446       -6

Impacted Files	Coverage Δ
pystac/serialization/__init__.py	`100.00% <ø> (+13.04%)`	⬆️
pystac/stac_io.py	`77.19% <83.33%> (+0.48%)`	⬆️
pystac/stac_object.py	`94.70% <87.50%> (-0.35%)`	⬇️
pystac/__init__.py	`100.00% <100.00%> (ø)`
pystac/catalog.py	`94.70% <100.00%> (+1.19%)`	⬆️
pystac/collection.py	`95.49% <100.00%> (+1.76%)`	⬆️
pystac/item.py	`96.98% <100.00%> (+0.31%)`	⬆️

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 a729cd2...b73ac4d. Read the comment docs.

matthewhanson · 2021-06-17T19:41:59Z

this looks good, I think the consolidating the duplicated functions addresses the concern @lossyrob had.

Will test this out with pystac-client in a few.

duckontheweb · 2021-06-17T19:46:55Z

tests/test_writing.py

-            cat = pystac.Catalog.from_file(href)
+            cat = pystac.read_file(href)
+            if not isinstance(cat, pystac.Catalog):
+                raise pystac.STACTypeError(f"File at {href} is not a Catalog.")


@lossyrob @matthewhanson One consequence of adding the STACObject.identify methods is that Catalog.from_file will now fail on a Collection file, which is why these lines changed. Judging from discussion in radiantearth/stac-spec#1082 I think this is the right behavior, but let me know if you feel otherwise. If we feel it should succeed, I can update Catalog.identify accordingly.

This seems good, though we should include the type information of what it is so that if someone does expect Catalog.form_file to work with collections, it will say "File at {href} is a {cat.STAC_OBJECT_TYPE } not a Catalog"

I think that's the right behavior. A Collection is not a Catalog, so should not be able to be opened up as such

lossyrob

Minor changes suggested, LGTM otherwise!

pystac/__init__.py

lossyrob · 2021-06-17T19:56:43Z

tests/test_writing.py

-            cat = pystac.Catalog.from_file(href)
+            cat = pystac.read_file(href)
+            if not isinstance(cat, pystac.Catalog):
+                raise pystac.STACTypeError(f"File at {href} is not a Catalog.")


This seems good, though we should include the type information of what it is so that if someone does expect Catalog.form_file to work with collections, it will say "File at {href} is a {cat.STAC_OBJECT_TYPE } not a Catalog"

pystac/catalog.py

lossyrob · 2021-06-17T22:06:45Z

@duckontheweb mind if I merge? Some changes I'm working on now for #453 should be based on these changes.

duckontheweb · 2021-06-17T22:37:59Z

No, go for it!

lossyrob

I actually just merged in the changes from this branch, and had a question about STACObject.from_file, comment inline

lossyrob · 2021-06-17T22:39:00Z

pystac/stac_object.py

        if stac_io is None:
            stac_io = pystac.StacIO.default()

        if not is_absolute_href(href):
            href = make_absolute_href(href)

-        o = stac_io.read_stac_object(href)
+        d = stac_io.read_json(href)
+        o = cls.from_dict(d, href=href, migrate=True)


Why this change? Couldn't this remain stac_io.read_stac_object for one codepath to handle href -> STACObject logic?

The type will be STACObject as far as type hinting goes, but the underlying class will be either a Catalog, Collection or Item. The casting done in the implementations should work out fine.

Just made this change and saw that some tests break with custom STACObjects. disregard.

Answered my own question

duckontheweb added 2 commits June 17, 2021 09:49

Use cls argument in from_dict methods

f1f02a2

Return calling class in from_file

6e0785b

duckontheweb requested review from matthewhanson and lossyrob June 17, 2021 15:00

duckontheweb mentioned this pull request Jun 17, 2021

[WIP] Improve STACObject inheritance #443

Closed

4 tasks

Simplify from_file logic

3f6b58e

Improve I/O test coverage

b73ac4d

matthewhanson approved these changes Jun 17, 2021

View reviewed changes

duckontheweb commented Jun 17, 2021

View reviewed changes

lossyrob approved these changes Jun 17, 2021

View reviewed changes

duckontheweb added 4 commits June 17, 2021 17:44

Add stac_io argument to read_file and write_file

4ebc660

Rename method for identifying dict type

e15c7c0

More specific error message in test_writing

1fbf4c2

Rename to matches_object_type

a6aa201

lossyrob previously requested changes Jun 17, 2021

View reviewed changes

lossyrob approved these changes Jun 17, 2021

View reviewed changes

lossyrob merged commit 2739661 into stac-utils:main Jun 17, 2021

duckontheweb deleted the fix/gh-410-stac-object-inheritance branch June 17, 2021 23:57

This was referenced Jun 18, 2021

IO updates, new StacApiIO class stac-utils/pystac-client#60

Merged

Exception if Catalog doesn't have a description field during catalog.get_children() #464

Closed

Casting root Catalog to Catalog type interferes with pystac-client Client class #410

Closed

duckontheweb mentioned this pull request Jul 2, 2021

Return calling class from Link constructor methods #512

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix STACObject inheritance #451

Fix STACObject inheritance #451

duckontheweb commented Jun 17, 2021 •

edited

Loading

duckontheweb commented Jun 17, 2021

lossyrob commented Jun 17, 2021

duckontheweb commented Jun 17, 2021

codecov-commenter commented Jun 17, 2021 •

edited

Loading

matthewhanson commented Jun 17, 2021

duckontheweb Jun 17, 2021

lossyrob Jun 17, 2021

matthewhanson Jun 17, 2021

lossyrob left a comment

lossyrob Jun 17, 2021

lossyrob commented Jun 17, 2021

duckontheweb commented Jun 17, 2021

lossyrob left a comment

lossyrob Jun 17, 2021

lossyrob Jun 17, 2021

Fix STACObject inheritance #451

Fix STACObject inheritance #451

Conversation

duckontheweb commented Jun 17, 2021 • edited Loading

duckontheweb commented Jun 17, 2021

lossyrob commented Jun 17, 2021

duckontheweb commented Jun 17, 2021

codecov-commenter commented Jun 17, 2021 • edited Loading

Codecov Report

matthewhanson commented Jun 17, 2021

duckontheweb Jun 17, 2021

Choose a reason for hiding this comment

lossyrob Jun 17, 2021

Choose a reason for hiding this comment

matthewhanson Jun 17, 2021

Choose a reason for hiding this comment

lossyrob left a comment

Choose a reason for hiding this comment

lossyrob Jun 17, 2021

Choose a reason for hiding this comment

lossyrob commented Jun 17, 2021

duckontheweb commented Jun 17, 2021

lossyrob left a comment

Choose a reason for hiding this comment

lossyrob Jun 17, 2021

Choose a reason for hiding this comment

lossyrob Jun 17, 2021

Choose a reason for hiding this comment

duckontheweb commented Jun 17, 2021 •

edited

Loading

codecov-commenter commented Jun 17, 2021 •

edited

Loading