Add deprecation warnings

[pjhartzell]

Related Issue(s):

• Closes Surface warnings when accessing deprecated collections #156 #843

Description:

• Inserts logic into the Item and Collection from_dict() methods that checks for the version extension and, if it exists, issues a DeprecatedWarning if the version extension deprecated field is set to True.
• Provides an ignore_deprecated context manager to suppress the DeprecatedWarning.

This PR originally used logic outside the version extension to check for the deprecated field on assets. This was removed in favor of working within the version extension's current deprecation checking ability.

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.

[gadomski]

The changes make sense to me, but I'm a little worried about the performance implications -- by looping through every asset on every serialization, we're adding some extra work to a very common operation. Does our current benchmark suite test an item with enough assets to pick up the change, and if so can we quantify that?

[pjhartzell]

but I'm a little worried about the performance implications

Yeah, me too. I forgot about our benchmark suite! I'll dig in and see what we can compare.

[pjhartzell]

I switched to the tests/data-files/eo/eo-sentinel2-item.json for the Item bench --> it has 17 assets. After running ./scripts/bench, the output from asv compare main HEAD for timing the item.from_dict() methods is below. I did this twice.

First Run:

       before           after         ratio
     [f78637a9]       [6a1b1cf7]
     <main>           <issues/843-deprecation-warnings>
          286±1μs          286±1μs     1.00  item.ItemBench.time_item_from_dict [Prestons-MacBook-Pro.local/virtualenv-py3.9]
          285±2μs          286±1μs     1.00  item.ItemBench.time_item_from_dict [Prestons-MacBook-Pro.local/virtualenv-py3.9-orjson]

Second Run:

       before           after         ratio
     [f78637a9]       [6a1b1cf7]
     <main>           <issues/843-deprecation-warnings>
          283±2μs          283±2μs     1.00  item.ItemBench.time_item_from_dict [Prestons-MacBook-Pro.local/virtualenv-py3.9]
          281±2μs          282±2μs     1.00  item.ItemBench.time_item_from_dict [Prestons-MacBook-Pro.local/virtualenv-py3.9-orjson]

No substantial effect, it seems.

[pjhartzell]

Some notes. @gadomski
Bug = Need to check if the deprecated field is True. Currently only checking for existence.
Perf = Only run deprecated checks if version extension exists in the stac_extensions list.
Refactor = Might it be cleaner (and more useful) to add an is_deprecated() method on the Asset, Collection, and Catalog classes? The deprecation checks in the from_dict() methods currently occur after a class has been created, so we would have access to the proposed is_deprecated() methods. We'd still have to loop, but the logic would be moved to dedicated methods.

[gadomski]

Looks good. It'd be nice to use the version extension instead of accessing "deprecated" directly -- if I recall correctly, we chatted offline about doing that first. Correct me if I'm wrong about that, @pjhartzell?

I think we should also add a section to the docs talking about deprecated objects and how to silence the warning.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (8e60096) 90.30% compared to head (4c83992) 90.34%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   90.30%   90.34%   +0.04%     
==========================================
  Files          47       47              
  Lines        6186     6213      +27     
  Branches      927      929       +2     
==========================================
+ Hits         5586     5613      +27     
  Misses        422      422              
  Partials      178      178              

Impacted Files	Coverage Δ
pystac/__init__.py	94.00% <ø> (ø)
pystac/collection.py	94.83% <100.00%> (+0.15%)	⬆️
pystac/errors.py	100.00% <100.00%> (ø)
pystac/extensions/version.py	96.55% <100.00%> (+0.25%)	⬆️
pystac/item.py	93.11% <100.00%> (+0.29%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pjhartzell]

@gadomski We now have a pared-down implementation that uses the Version extension to check only Items and Collections for deprecation. Contains a new get_version method on the extension mixin class.

Just want to raise a flag that adding get_version may be opening a can of worms. It causes one to start looking at how the other extension methods do not account for differing versions.

• For example, if your item contains a different version than in SCHEMA_URI, you can have two versions of the same extension in stac_extensions via a call to the add_to method.
• The remove_from method only works for the hard-coded version in SCHEMA_URI
• validate_has_extension only checks for the version in SCHEMA_URI.

We're opening the door to the reality that different extension versions exist and may be present on a STAC object, but there is some decent refactor required to handle that reality.

[gadomski]

TIL recwarn, I like that a lot! Sorry for being a 🧇 on the has_extension stuff -- if we don't keep the get_version changes in this PR, let's make sure to put it into another branch/draft PR so we don't lose it, it's useful stuff.

[pjhartzell]

I think we should also add a section to the docs talking about deprecated objects and how to silence the warning.

Sounds good. I put a note on the deprecated property of the version extension with an example.

[gadomski]

Thanks for making the change to try/except, that reads much cleaner to me. I did a local benchmark run and no significant change. I also added another test section to see what happens when there's no extension url -- works fine.