Add support for the version extension. #193

schwehr · 2020-10-09T22:07:13Z

The initial version only implements version for Items. I still need to do version for Collections. Also, I'm sure more tests are needed.

https://github.com/radiantearth/stac-spec/tree/dev/extensions/version

schwehr · 2020-10-09T22:35:59Z

I will fix this:

flake8 pystac tests
pystac/extensions/version.py:24:7: E111 indentation is not a multiple of four

schwehr · 2020-10-10T17:46:33Z

I will try to fix the next linter's complaints later today.

Reverted the changes to pystac/validation/stac_validator.py yapf3 --version yapf 0.29.0

schwehr · 2020-10-10T20:59:52Z

I need to add a test that validates an item created with a version

codecov-io · 2020-10-10T21:00:58Z

Codecov Report

Merging #193 into develop will increase coverage by 0.37%.
The diff coverage is 98.30%.

@@             Coverage Diff             @@
##           develop     #193      +/-   ##
===========================================
+ Coverage    91.81%   92.18%   +0.37%     
===========================================
  Files           27       28       +1     
  Lines         3225     3341     +116     
===========================================
+ Hits          2961     3080     +119     
+ Misses         264      261       -3

Impacted Files	Coverage Δ
pystac/extensions/version.py	`98.26% <98.26%> (ø)`
pystac/__init__.py	`100.00% <100.00%> (ø)`
pystac/stac_object.py	`94.89% <0.00%> (+0.51%)`	⬆️
pystac/catalog.py	`89.93% <0.00%> (+1.38%)`	⬆️

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 9944c51...fbd7abd. Read the comment docs.

schwehr · 2020-10-10T21:56:49Z

I will add validation code like this when I get a chance.

  def testValidateMinimum(self):
    with self.assertRaises(pystac.validation.STACValidationError):
      self.item.validate()

    self.item.ext.version.apply(self.version)
    self.item.validate()

  def testValidateAll(self):
    deprecated = True
    latest = MakeItem(2013)
    predecessor = MakeItem(2010)
    successor = MakeItem(2012)
    self.item.ext.version.apply(self.version, deprecated, latest, predecessor, successor)

    self.item.validate()

It appears that I need to add geometry to the item for it to be valid. (or remove the bbox and geometry)

def MakeItem(year):
  asset_id = f'USGS/GAP/CONUS/{year}'
  bbox = [-160.26, 18.85, -154.66, 22.29]
  geometry = {
      'type': 'Polygon',
     # TODO(schwehr): Match coords
      'coordinates': [[
          [100.0, 0.0],
          [101.0, 0.0],
          [101.0, 1.0],
          [100.0, 1.0],
          [100.0, 0.0]]]}
  start = datetime.datetime(year, 1, 2)

  item = pystac.Item(
      id=asset_id, geometry=geometry, bbox=bbox, datetime=start, properties={})
  item.set_self_href(URL_TEMPLATE % year)

  item.ext.enable(pystac.Extensions.VERSION)

  return item

Stop using an actual bbox and set it to None as geometry and bounds are not relevant to testing the version extension. Also add a test case with all three of latest, predecessor, and successor.

schwehr · 2020-10-11T05:19:50Z

Probably best to add a test where the deprecated arg to apply was not a bool or None, but I can do that after the rest of version gets written.

lossyrob · 2020-10-13T14:18:32Z

pystac/extensions/version.py

+
+    @classmethod
+    def _object_links(cls):
+        return []  # TODO(schwehr): What should this return?


This method returns any STAC Objects that are linked to by the extension properties. That way, PySTAC knows that if can resolve links and should check if a link to a referenced object exists in the resolved object cache.

In this case, this should return any of the successor, predecessor, and latest_version links, e.g.

Suggested change

return [] # TODO(schwehr): What should this return?

link_types = [SUCCESSOR_VERSION, PREDECESSOR_VERSION, LATEST_VERSION]

return [link for link in self.item.get_links() if link.rel in link_types]

What call triggers this? This already returns the linked item

print('objects latest', [x for x in self.item.get_stac_objects(version.LATEST_VERSION)])

Your example has self.item, but this is a @classmethod with cls as the arg. I'm not following.

My bad, forgot this was a classmethod. It should return the rel types that point to stac objects, not the actual links.

So return [SUCCESSOR_VERSION, PREDECESSOR_VERSION, LATEST_VERSION] should work for this case.

Is there a reasonably direct way to test this? I switched the order the be alphabetical. If there was a specific reason for the order you listed, I will change it.

I'm going to look at the catalog copy test now.

No reason for the ordering, so alphabetical is great!

Not a very direct test; the full_copy logic is really what this is used for so the more complicated test is what will really ensure its working.

pystac/extensions/version.py

tests/extensions/test_version.py

lossyrob · 2020-10-13T14:37:05Z

tests/extensions/test_version.py

+        predecessor = MakeItem(2010)
+        successor = MakeItem(2012)
+        self.item.ext.version.apply(self.version, deprecated, latest, predecessor, successor)
+        # Just validate in tearDown.


Once the _object_links method is implemented, there's a somewhat tricky test that should be performed that will make sure the in-memory object instance tracking works for this extension. The test could go as follows:

Read in a test case catalog a la https://github.com/stac-utils/pystac/blob/develop/tests/test_catalog.py#L194

Enable the version extension on some or all of the items (or collections)

Link some of the items together as 'prev_version', etc. E.g. Item A links to Item B with the 'predecessor-version' rel type.

Make a full copy of the catalog

Check to see that there aren't duplicates made. In the example above, that means that if I retrieve Item A and Item B from the catalog, and then I do:

link = item_a.get_link('predecessor-version'`).resolve_stac_object() self.assertTrue(link.target is item_b)

That the test passes. So we're checking that the copy didn't make one copy for Item B as it exists in the catalog, and another copy for the link from Item A to Item B with the version extension.

What I have so far:

def test_copy_1_with_version(self): c2011 = make_collection(2011) c2012 = make_collection(2012) c2013 = make_collection(2013) c2014 = make_collection(2014) c2012.ext.enable(pystac.Extensions.VERSION) c2012.ext.version.apply('v2012', latest=c2012, predecessor=c2011, successor=c2013) cat = pystac.Catalog('my_catalog', 'my description') cat.add_child(c2011) cat.add_child(c2012) cat.add_child(c2013) cat.add_child(c2014) cat_copy = cat.full_copy()

Was trying to figure out how best to compare the two. Hopefully I can think through it easier where there aren't mindcraft videos playing next to me... :|

Having a hard time figuring out the item link thing. This is what I had as of the end of yesterday

def compare_collection(col1, col2): if col1.id != col2.id: return False if col1.description != col2.description: return False has_ver_1 = col1.ext.implements(pystac.Extensions.VERSION) has_ver_2 = col2.ext.implements(pystac.Extensions.VERSION) if has_ver_1 != has_ver_2: return False if has_ver_1: print('has_ver') ver1 = col1.ext.version ver2 = col2.ext.version if ver1.version != ver2.version: return False if ver1.deprecated != ver2.deprecated: return False if str(ver1.latest_link) != str(ver2.latest_link): return False if str(ver1.predecessor_link) != str(ver2.predecessor_link): return False if str(ver1.successor_link) != str(ver2.successor_link): return False return True class FullCopyTest(googletest.TestCase): def compare_catalogs(self, cat1, cat2): self.assertEqual(cat1.id, cat2.id) children1 = {child.id: child for child in cat1.get_children()} # print('children1 dict', children1) children2 = {child.id: child for child in cat2.get_children()} # print('children2 dict', children2) self.assertCountEqual(sorted(children1.keys()), sorted(children2.keys())) for child_id in children1: print(child_id) child1 = children1[child_id] child2 = children2[child_id] has_ver_1 = child1.implements(pystac.Extensions.VERSION) has_ver_2 = child1.implements(pystac.Extensions.VERSION) self.assertEqual(has_ver_1, has_ver_2) if has_ver_1: # Umm... ? self.assertTrue(compare_collection(child1, child2)) link = child1.get_links(version.LATEST_VERSION)[0].resolve_stac_object() self.assertIs(link.target, child2) return True def testCopy1Linked(self): c2011 = make_collection(2011) c2012 = make_collection(2012) c2013 = make_collection(2013) c2014 = make_collection(2014) c2012.ext.enable(pystac.Extensions.VERSION) c2012.ext.version.apply( 'v2012', latest=c2014, predecessor=c2011, successor=c2013) cat = pystac.Catalog('my_catalog', 'my description') cat.add_child(c2011) cat.add_child(c2012) cat.add_child(c2013) cat.add_child(c2014) cat_copy = cat.full_copy() pprint(cat_copy.to_dict()) self.compare_catalogs(cat, cat_copy)

The easier thing would be to do a straight object identity test to ensure the object cache ensures there's a single object that's linked to in the catalog and by the item that has the version link.

It might be easier/more clear to make the suggestion in code. I can take a crack at this tomorrow if you're alright with me pushing a commit to this branch?

Go for it! The only things I really care about is learning how it's done and getting the extensions implemented. I don't like what I did above except for the setup (assuming that it makes input of the form required.)

Running validate in tearDown happens after the test results are recorded, so validate should happen in the tests. This simplifies the tests where validate doesn't really make sense. Also removed a redundant call to self.collection.ext.enable.

schwehr · 2020-10-13T19:18:56Z

Doh. Committed the wrong thing, so I backed up and forced the commit.

lossyrob · 2020-10-15T15:21:49Z

tests/extensions/test_version.py

@@ -104,6 +105,39 @@ def test_all_links(self):
        self.item.ext.version.apply(self.version, deprecated, latest, predecessor, successor)
        self.item.validate()

+    def test_full_copy(self):


@schwehr here are the two tests that I used to make sure the object links were operating correctly in a full_copy scenario.

Notice that if you don't return the correct reltypes from the _object_link method, these tests fail.

This is because, when making a full copy of the catalog in memory, PySTAC doesn't know that those links point to STAC objects that should be considered in the copy. In this bit of code, the logic to determine whether or not a link has to be copied (and it's target either copied or pulled from the cache of already-copied objects) is determined by the _object_links method, which as seen here depends on object link the Catalog or Item declares (core reltypes like child, item or collection), as well as any reltypes that are declared to be STAC objects by implemented extensions.

Let me know if this helps explain the _object_links method and why this is a good way to test that, or feel free to make changes or give ideas on way to improve!

Arg... I'm a noob and forced commit over your change. I'll recommit it from this :(

No worries! I find git cherry-pick to be the most useful in those types of situations.

Thanks! I spend most of my time in piper where cherry-picks are usually just for release branches. Appreciate any help leveling up my git skills.

This commit adds two tests, one for each of the Item and Collection extensions of the Version extension. The test checks that when items or collections are linked by the version links (predecessor, successor, or latest_version), that a full copy will end up with a catalog that has those links point to the correct object instances. If this test were to fail, that would indicate that the STAC object linked to by the version links was a separate copy of item or collection, which is incorrect. Redo of stac-utils@defd7ef by lossyrob

pystac/extensions/version.py

Suggested by lossyrob@

@lossyrob

…ion. Suggested by @lossyrob Also: - Add VERSION and DEPRECATED to avoid using bare strings - Removed extra blank lines - Added year to id set in make_collection for easier debugging

schwehr · 2020-10-15T18:33:09Z

Looks like this was a flake. Not able to read the schema, yes?

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/jsonschema/validators.py", line 777, in resolve_from_url
    document = self.resolve_remote(url)
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/jsonschema/validators.py", line 860, in resolve_remote
    result = requests.get(uri).json()
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/requests/models.py", line 889, in json
    return complexjson.loads(
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/json/__init__.py", line 357, in loads
    return _default_decoder.decode(s)
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

lossyrob

One small, more stylistic change - feel free to push back on it if you're used to that method :-)

Other then that this is looking 👍 and I'll merge after we sort out this last bit. Thank you!

lossyrob · 2020-10-16T13:12:07Z

pystac/extensions/version.py

+        if items:
+            return next(items)


One last little nitpick -

Making the return in the None case would be more explicit here and elsewhere, rather than not returning and relying on Python's default mechanism to serve None back to the caller.

Here and in the other iterator cases you could combine these lines into:

Suggested change

if items:

return next(items)

return next(items, None)

I think it'd still be readable if you even one-lined it:

return next(self.item.get_stac_objects(SUCCESSOR), None)

Awesome! I haven't looked at next() in ages. The default None is so much simpler.

Done in schwehr@974dac8

The docs are nice an on point... https://docs.python.org/3/library/functions.html#next

lossyrob · 2020-10-16T13:31:04Z

One more thing I forgot to mention - the next extension needs an entry in the API docs. We use autodoc but it requires we specify the class, and then give it a heading so it appears nicely in the TOC. See this for an example.

@lossyrob

Suggestion by @lossyrob

Also fix the Point Cloud Extension that had copy-paste remnants from projection.

schwehr · 2020-10-16T14:05:26Z

One more thing I forgot to mention - the next extension needs an entry in the API docs. We use autodoc but it requires we specify the class, and then give it a heading so it appears nicely in the TOC. See this for an example.

Cool. Done in fbd7abd. The order in the API doc is a bit confusing. I did fix up some complains from the tooling about the Point Cloud Extension.

lossyrob · 2020-10-16T14:24:00Z

Thanks for fixing the pointcloud docs, I hadn't noticed that that had been breaking the doc build.

Looks great, merging!

Add support for the version extension.

67a3109

version.py: Fix indenting issue found by flake8

36b4183

Fix formatting with yapf3 --in-place -pr pystac tests

8753317

Reverted the changes to pystac/validation/stac_validator.py yapf3 --version yapf 0.29.0

test_version.py: Validate self.item during tearDown.

4db4048

Stop using an actual bbox and set it to None as geometry and bounds are not relevant to testing the version extension. Also add a test case with all three of latest, predecessor, and successor.

schwehr mentioned this pull request Oct 11, 2020

Support the scientific extension #194

Closed

version.py: Add VersionCollectionExt

a5590e9

lossyrob reviewed Oct 13, 2020

View reviewed changes

schwehr added 2 commits October 13, 2020 11:15

version.py: Implement _object_links.

c34eeb1

schwehr force-pushed the version-extension branch from 9eff9a2 to c34eeb1 Compare October 13, 2020 19:18

lossyrob reviewed Oct 15, 2020

View reviewed changes

schwehr mentioned this pull request Oct 15, 2020

Add support for the scientific extension. #199

Merged

Merge test_full_copy by lossyrob

beac9dd

schwehr force-pushed the version-extension branch from defd7ef to beac9dd Compare October 15, 2020 16:18

lossyrob requested changes Oct 15, 2020

View reviewed changes

pystac/extensions/version.py Outdated Show resolved Hide resolved

schwehr added 3 commits October 15, 2020 10:15

version.py: Remove _link from method names

b35cd64

Suggested by lossyrob@

version: Drop redundant _VERSION from field name constants

8ea0f91

version: Switch from returning a link to returning an Item or Collect…

d408d2e

…ion. Suggested by @lossyrob Also: - Add VERSION and DEPRECATED to avoid using bare strings - Removed extra blank lines - Added year to id set in make_collection for easier debugging

test_version.py: Use version.VERSION and version.DEPRECATED

56e9033

lossyrob requested changes Oct 16, 2020

View reviewed changes

schwehr added 2 commits October 16, 2020 06:48

version.py: Use next() default value of None

974dac8

Suggestion by @lossyrob

api.rst: Add Version Extension.

fbd7abd

Also fix the Point Cloud Extension that had copy-paste remnants from projection.

lossyrob approved these changes Oct 16, 2020

View reviewed changes

lossyrob merged commit eaf8b42 into stac-utils:develop Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the version extension. #193

Add support for the version extension. #193

schwehr commented Oct 9, 2020 •

edited

Loading

schwehr commented Oct 9, 2020

schwehr commented Oct 10, 2020

schwehr commented Oct 10, 2020

codecov-io commented Oct 10, 2020 •

edited

Loading

schwehr commented Oct 10, 2020

schwehr commented Oct 11, 2020

lossyrob Oct 13, 2020

schwehr Oct 13, 2020

lossyrob Oct 13, 2020

schwehr Oct 13, 2020

lossyrob Oct 13, 2020

lossyrob Oct 13, 2020

schwehr Oct 13, 2020

schwehr Oct 14, 2020

lossyrob Oct 14, 2020

schwehr Oct 14, 2020

schwehr commented Oct 13, 2020

lossyrob Oct 15, 2020

schwehr Oct 15, 2020

lossyrob Oct 15, 2020

schwehr Oct 15, 2020

schwehr commented Oct 15, 2020

lossyrob left a comment

lossyrob Oct 16, 2020

schwehr Oct 16, 2020 •

edited

Loading

lossyrob commented Oct 16, 2020

schwehr commented Oct 16, 2020 •

edited

Loading

lossyrob commented Oct 16, 2020

	return [] # TODO(schwehr): What should this return?
	link_types = [SUCCESSOR_VERSION, PREDECESSOR_VERSION, LATEST_VERSION]
	return [link for link in self.item.get_links() if link.rel in link_types]

Add support for the version extension. #193

Add support for the version extension. #193

Conversation

schwehr commented Oct 9, 2020 • edited Loading

schwehr commented Oct 9, 2020

schwehr commented Oct 10, 2020

schwehr commented Oct 10, 2020

codecov-io commented Oct 10, 2020 • edited Loading

Codecov Report

schwehr commented Oct 10, 2020

schwehr commented Oct 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schwehr commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schwehr commented Oct 15, 2020

lossyrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schwehr Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

lossyrob commented Oct 16, 2020

schwehr commented Oct 16, 2020 • edited Loading

lossyrob commented Oct 16, 2020

schwehr commented Oct 9, 2020 •

edited

Loading

codecov-io commented Oct 10, 2020 •

edited

Loading

schwehr Oct 16, 2020 •

edited

Loading

schwehr commented Oct 16, 2020 •

edited

Loading