RFC - Caching of non-Flyte offloaded objects

[eapolinario]

#1581

[EngHabu]

I think a big question for me is whether it should be opt-in for certain types... like Pandas (for which data has already been loaded in memory)... I understand that the proposal, at least from a purity perspective, leaves it as opt-in feature for all... but I would like to question that a bit... would it be a fair assumption, on users side, to say that "built-in" types are always by value?

[eapolinario]

would it be a fair assumption, on users side, to say that "built-in" types are always by value?

That's true, but it happened almost by accident, i.e. we never exposed (nor enforced) the notion that values were being cached by value or by reference.

Also, we shouldn't be too prescriptive about hashing complex types for two reasons:

1. cost: calculating hashes might be expensive
2. No single way of calculating the hash.
   i. We can certainly offer a few default implementations (and be upfront about the probability of collisions, etc)

Does this make sense? Can you clarify what we could gain if we made this assumption of all types will be cached by value the default?

[EngHabu]

Can you clarify what we could gain if we made this assumption of all types will be cached by value the default?

Mainly that from the feedback we heard a few times (and maybe it's overblown) that this was what the existing UX suggested to them... the reason I want to discuss this now is if we decide to offer it by default to some types, we will need an opt-out experience... and I think it might be worth writing down a few examples of each....

[eapolinario]

This is the first time we're even making the distinction between the two caching semantics. I think it's fair to assume (and make it clear in the docs) that we use cache-by-value everywhere unless specified, in which case users will be able to opt-in to this experience of providing cache-by-value semantics for specific types.

[EngHabu]

I think as part of this work we should enrich the caching data sent by propeller to indicate whether caching by value or reference occurred... not sure of the specific syntax but conveying this information, I believe, is important...

Specially in the scenario where you call like this:

@workflow
def wf1() -> Something:
  df = query_data() # produces hash-annotated dataframes
  return expensive_compute(df)

@workflow
def wf2(df: pd.DataFrame) -> Something:
  expensive_compute(df)

In which case, it's very plausible that wf2() gets called with non-hash-annotated DataFrame (maybe called from the UI with an s3 path) and will NOT match the cache key produced/used within wf1() execution... and conveying this distinction/expectation to users I think is important to avoid confusion..

[eapolinario]

That's a really good catch.

Do you think it's worth annotating the type in the declaration of expensive_compute, as having the hash overridden? Something like:

def expensive_compute(df: Annotated[pd.DataFrame, HashOverriden]):
   ...

This way, callers of expensive_compute might see in the logs if they call that task with dataframes missing the hash annotation.

[EngHabu]

Not sure what does it mean to have an input with that annotation? do you mean flyte/flytekit would log that "task author expected hash and you provided non..." ?

[eapolinario]

do you mean flyte/flytekit would log that "task author expected hash and you provided non..." ?

Exactly.

[EngHabu]

If we leverage tags as we seem to be leaning towards, maybe this is a moot point then? because by reference lookup will continue to work as expected? I think if we surface the information to the user (that we used did a lookup by reference not by hash) maybe that is enough? not as a warning or anything... just stating a fact...
We will have to thoroughly document all of this... it's a good pattern if we can make sure users understand what's going on...

[eapolinario]

I still think that it's worth showing to the user whenever we are able that this caching-by-value semantics is used.

One idea we talked about is to catch cases as early as during registration phase and at least inform the user that their intent might not match the code. I updated the RFC with two examples of such ideas.

[EngHabu]

Awesome! LGTM