V2: get_target() while recursing items results in multiple instances for same object

[tylanderson]

Calling get_target() on a link (same link href, different object) while recursing through items (e.g. col.get_items()) will result in a new instance of that targeted object. This is a change from v1

An simplified example of this, adapted from stactools fixture (where get_parent() calls get_target()):

col = pystac.Collection.from_file(
        "tests/data-files/planet-disaster/collection.json"
)
for item in list(col.get_items(recursive=True)):
    parent = item.get_parent()
    print(f"Parent is {parent.id}: {id(parent)}")

will yield

Parent is hurricane-harvey-0831: 4512587968
Parent is hurricane-harvey-0831: 4599873760
Parent is hurricane-harvey-0831: 4512212864
Parent is hurricane-harvey-0831: 4512213728
Parent is hurricane-harvey-0831: 4512214640

where each item creates a new instance of the parent. This broke a test fixture where mutating the parent from one item updated the same instance that other items accessed.

I'm not sure if this behavior from v1 (cached) is what we want to keep for v2, but surfacing.

[gadomski]

I'm not sure if this behavior from v1 (cached) is what we want to keep for v2, but surfacing.

Appreciate you surfacing, and this is exactly the sort of thing that I want to find and either (a) fix or (b) explicitly call out as breaking for v2.

My instinct is to intentionally break this for v2 — the whole caching layer in pystac is a huge PITA to maintain, for what is (IMO) quite minimal benefit. Most systems I've seen in the wild aren't keeping large STAC trees in memory and doing complicated things with them, which is the main case when you'd care about caching references to the same object. Most uses-cases are either reading (you only touch things once) or writing (where you might create-then-mutate).

[tylanderson]

Most uses-cases are either reading (you only touch things once) or writing (where you might create-then-mutate).

Yeah in this case it was only used in a test fixture to used to test merging, which can be rewritten easily as long as you understand how the v2 changes here, so calling out would be helpful for anyone else who used it in this way

[gadomski]

Thinking about it this morning, I do wonder where a "cached reader" could be a reasonable compromise here — make the default pystac reader store a bounded cache from href to STAC value.

[jsignell]

Do you think it would be enough to put a cache decorator on the function col.get_items so that it memoizes the mapping? Or is that too unpredictable?

[gadomski]

Or is that too unpredictable?

I think it's too unpredictable, because you might get to the same object some other way (e.g. through a sibling) and it wouldn't be the same?