Support `recursive` with `get_items`

[jsignell]

I was running tests locally with dev install of pystac and pystac-client and I got a test failure on TestCollectionClient.test_get_item_with_item_search:

self = <tests.test_collection_client.TestCollectionClient object at 0x7f6accfde190>

    @pytest.mark.vcr
    def test_get_item_with_item_search(self) -> None:
        client = Client.open(STAC_URLS["PLANETARY-COMPUTER"])
        collection = client.get_collection("aster-l1t")
        assert collection is not None
        assert client._stac_io
        client._stac_io.set_conformance(
            [
                "https://api.stacspec.org/v1.0.0-rc.2/core",
                "https://api.stacspec.org/v1.0.0-rc.2/item-search",
            ]
        )
        item = collection.get_item("AST_L1T_00312272006020322_20150518201805")
        assert item
        assert item.id == "AST_L1T_00312272006020322_20150518201805"
    
        item = collection.get_item("for-sure-not-a-real-id")
        assert item is None
    
>       item = collection.get_item(
            "AST_L1T_00312272006020322_20150518201805", recursive=True
        )

tests/test_collection_client.py:58: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pystac_client/collection_client.py:171: in get_item
    item = super().get_item(id, recursive=True)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <CollectionClient id=aster-l1t>, id = 'AST_L1T_00312272006020322_20150518201805'
recursive = True

    def get_item(self, id: str, recursive: bool = False) -> Optional[Item]:
        """Returns an item with a given ID.
    
        Args:
            id : The ID of the item to find.
            recursive : If True, search this collection and all children for the
                item; otherwise, only search the items of this collection. Defaults
                to False.
    
        Return:
            Item or None: The item with the given ID, or None if not found.
        """
>       return next(self.get_items(id, recursive=recursive), None)
E       TypeError: CollectionClient.get_items() got an unexpected keyword argument 'recursive'

This is the result of an internal change in pystac: stac-utils/pystac#1075

Questions:

1. Should we deprecate get_all_items and get_item like we did in pystac?
2. Do we need to support old versions of pystac in new versions of pystac-client?
3. Are there any upstream tests that install pystac off main? And if not would it be helpful to set some up?
4. Should we revert the change in pystac and save it for 2.0?

[gadomski]

tl;dr: ugh, inheritance

Should we deprecate get_all_items and get_item like we did in pystac?

get_all_items, yes. The current get_all_items just wraps get_items anyways.

get_item I'm not sure, and I'm actually second-guessing our decision to deprecate it in PySTAC. That seems like a useful method, no? I'll open a issue to discuss over there so we don't cross concerns.

EDIT: I forgot that we kept Collection.get_item and just deprecated Catalog.get_item, since item ids are not required to be unique across catalogs. Now I think we should deprecate get_item here as well (pending the "should we revert until pystac v2" discussion).

Do we need to support old versions of pystac in new versions of pystac-client?

I don't think so? Since we're pretty dependency-light in pystac, raising the pystac>=X.Y.Z floor in pystac_client should be relatively low-risk for downstreams.

Are there any upstream tests that install pystac off main? And if not would it be helpful to set some up?

👍🏼, shouldn't block merging PRs though IMO.

Should we revert the change in pystac and save it for 2.0?

Interesting question. I had never thought that adding an optional argument w/ a default would be breaking, but because of inheritance I guess it is. I might need to think about this one a bit.

Thanks for flagging all this!! 🙇🏼

[jsignell]

The biggest question seems to be whether or not to revert the change in pystac. I think if we are ok with having a tight floor in pystac_client then we don't have to revert. The only issue I guess is that we would want to release them at the same time (or pystac_client first) since the floor will only be set on the pystac_client side.
.

Are there any upstream tests that install pystac off main? And if not would it be helpful to set some up?

👍🏼, shouldn't block merging PRs though IMO.

Yeah totally agree that just leads to a path of it being ok to merge with certain tests red, which is not great

[gadomski]

The biggest question seems to be whether or not to revert the change in pystac. I think if we are ok with having a tight floor in pystac_client then we don't have to revert. The only issue I guess is that we would want to release them at the same time (or pystac_client first) since the floor will only be set on the pystac_client side.

I'm concerned about Hyrum's law, aka that other people might be subclassing pystac.Catalog and have the same breakage? I think I'm leaning towards a conservative approach and reverting the change in pystac until v2.

Yeah totally agree that just leads to a path of it being ok to merge with certain tests red, which is not great

100% agree. It could be a forward-looking job that only fires on merge to main? That way PRs stay green, but we still get a notification if we're getting out-of-sync?

[ircwaves]

... I think I'm leaning towards a conservative approach and reverting the change in pystac until v2.

Yeah totally agree that just leads to a path of it being ok to merge with certain tests red, which is not great

100% agree. It could be a forward-looking job that only fires on merge to main? That way PRs stay green, but we still get a notification if we're getting out-of-sync?

I like the job that only fires on main approach. Or you could xfail with a conditional that would keep PRs green, and main red.

pytest.mark.xfail(condition=branch_is_not_pystac_main, "letting this ride until v2")

[gadomski]

👎🏼 on branch-specific config in Python code, I find that gets weird quickly. Would prefer to lean solely on Github Actions for the branch stuff.

[jsignell]

Yeah I can stand up a conditional github action job.

[jsignell]

I think the work left to do here is reverting the changes to pystac. Let me know if you want me to pick that up @gadomski

[gadomski]

Yeah, I think that's the right thing to do, go for it. Thanks!

[jsignell]

Well actually I am going to close this and open a ticket on pystac for the reversion