Removing the need to interact with Output in Assets

[alangenfeld]

Along the path to to shift IO management to opt-in, a road bump in the learning curve for users is that in order to communicate metadata about an asset materialization, they need to interact with the concepts of "outputs".

This is done via Output

@asset
def example():
    return Output(
        None,
        metadata={"one": 1},
    )

or via the context

@asset
def example(context: AssetExecutionContext):
    context.add_output_metadata(
        metadata={"one": 1},
    )

This is a bit awkward as we need to engage with the potentially new concept of "outputs" and fill out the value with None.

How do we improve this?

Option 1 context.add_asset_metadata

A slight tweak to the status quo - we add a similar method to add_output_metadata that avoids the concept of outputs.

• Pros: easy change
• Cons: uses context, poor testability

Option 2A return AssetMaterialization (implicit key)

Explored in #14931 we could allow returning materialization events directly.

@asset
def example(context: AssetExecutionContext):
    return AssetMaterialization(
        metadata={"one": 1},
    )

The AssetMaterialization returned would get merged with system provided information to create the final AssetMaterialization event that gets reported.

For convenience We make asset_key an optional argument in the single asset case by indirectly inferring it from the current context.

• Pros: testable, existing type
• Cons: complexity around asset key resolution

Option 2B return AssetMaterialization (explicit key)

A variant of 2A removing the shenanigans

@asset
def example(context: AssetExecutionContext):
    return AssetMaterialization(
       asset_key=example.__name__,
       metadata={"one": 1},
    )

• Pros: testable, existing type
• Cons: have to keep asset key aligned

Option 3 return MaterializationMetadata

A variant of 2, we could create a new type that users return for this purpose

@asset
def example(context: AssetExecutionContext):
    return MaterializationMetadata(
        metadata={"one": 1}
    )

• Pros: testability
• Cons: new noun

[alangenfeld]

I've been leaning Option 2 so far since I don't like adding a new type / noun.

[schrockn]

I add con to Option 2 there there is complexity around partition key as well.

[alangenfeld]

what is the partition key related problem that wouldn't apply in the other options?

[dpeng817]

worth noting that the partition key complexity has the added benefit of setting us up to yield dynamic partitions easily

[sryza]

For convenience We make asset_key an optional argument in the single asset case by indirectly inferring it from the current context.

I'm generally wary of these kinds of shenanigans, because they make it difficult for people to understand what's going on when they scan the code. And there are situations that become extra weird / friction-y. E.g.

def my_helper_function(...) -> AssetMaterialization:
    return AssetMaterialization(metadata=...)    

@asset
def my_asset():
    return my_helper_function(...)

def test_my_helper_function():
    result = my_helper_function(...)
    assert result == ???

[alangenfeld]

split to 2A / 2B in case its worth consider the re-use AssetMaterialization path without the shenanigans

[slopp]

I'd personally prefer option 1. I think for some users the concept of AssetMaterialization is opaque, and this "merging" sounds very confusing to reason about

The AssetMaterialization returned would get merged with system provided information to create the final AssetMaterialization event that gets reported.

I think context.add_output_metadata has been relatively easy to explain and changing the term to asset just makes it that much better.

[yuhan]

Option 1

Another thing that isn't great about the status-quo context.add_output_metadata is that its implementation mutates the output_metadata dict (code). IMO, Ideally for the metadata system, it would be something log-like, such as context.log_asset_metadata which emits a metadata event entry which doesn't mutate the state of output_metadata during a step execution. If we're going with the immutable log-like solution, I feel this basically leads to Option 2/3.

Option 2/3

I like the general direction of where this is going. Thinking from the docs perspective, eventually the asset concept page could be something like:

A software-defined asset includes the following:

• An AssetKey, which is a handle for referring to the asset.
• A set of upstream asset keys, which refer to assets that the contents of the software-defined asset are derived from.
• (assuming we'd like to de-emphasize the op implementation behind assets) An op, which is a function responsible for computing the contents of the asset from its upstream dependencies.
• A function responsible for computing the asset. The function can record Dagster-aware activities around the asset by returning (or yielding) one of the following: Output, AssetMaterialization, AssetObservation, AssetExpectation(maybe?).

[PedramNavid]

Just a small point against Context:

Context feels overloaded already for me as a user. I never know what is and isn't in Context (or what the correct type of Context should be at a given point (OpExecutionContext, AssetExecutionContext?) If I don't give context the right type (or an explicit type), then the IDE doesn't surface the methods.

[petehunt]

What's wrong with returning an Output? Seems to me like the API is pretty nice, even if the name Output is a bit odd. I suspect if we constrain the return types of assets to be either None or Output[T] (in effect, removing the ability to return a value that's not wrapped in Output, and removing generators from the API) it'll improve the discoverability of this API too.

[alangenfeld]

I updated the discussion to use return Output instead of yield, I was under the ancient / wrong understanding that we enforced more rules around when we permitted that.

[alangenfeld]

@slopp you have thoughts here? Sounds like maybe you point folks towards context.add_output_metadata, were you aware that return Output(None, metadata={...}) worked as well?

[schrockn]

I think the addition of Output adds another noun that is confusing if you are not thinking about asset in terms of i/o managers. Now you are in the world where you have to invent "output names" that are different from asset keys, and so forth.

[slopp]

Yea I guess my thinking is that:

• as a user, I pretty much always log stuff, so I've already overcome the learning curve for context.log.info("my message")
• in this situation, context.add_output_metadata isn't a giant leap

But both of those are true without me having ever

• used AssetMaterialization (TBH I'm still not really sure I could explain correctly the nuance of AssetMaterialization vs Output - I guess one is like a metadata event and the other is some I/O event?)
• used Output ^ see above. also Output(None) really isn't fun, cause who wants to deal with None. Wait till someone tries Output(Nan)
• touched asset keys, which I pretty much always mess up. Is it AssetKey(["schema", "table"]), or AssetKey("schema", "table"), or just asset_key = "string" or asset_key = this_function_name.__name__ 😕 or asset_key = AssetSelection().whole_new_world_of_potential_confusion

I'd rather just get into the habit of always typing the context (even if that is, to Pedram's point, sometimes annoying) than tackle ^.

Btw the reason typing the context is a little annoying is that I don't always know what the context type should be, and we don't do a great job in our docs of surfacing AssetExecutionContext vs SensorExecutionContext etc.

[alangenfeld]

The latest update based on design progress on the data quality work is to support MaterializeResult which is conceptually sibling to AssetCheckResult, PR here #15932