feat(looker): convert looker assets to external assets

[rexledesma]

Summary & Motivation

Convert the AssetsDefinition returned from @looker_assets to external assets. We do this manually rather than rely on external_assets_from_specs, since the definitions can be fit into one @multi_assets, rather than separate @multi_assets per spec.

Also, this gives us some flexibility in the future if some computation wants to be bundled with these external assets to turn them executable.

How I Tested These Changes

pytest, local

[rexledesma]

• feat(looker): convert looker assets to external assets #22322 👈
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rexledesma and the rest of your teammates on Graphite

[benpankow]

We do this manually rather than rely on external_assets_from_specs, since the definitions can be fit into one @multi_assets, rather than separate @multi_assets per spec.

Is there a meaningful difference between the two, since they can't be executed right now? I don't have a strong feeling either way, but would be nice to not duplicate logic w/ external_assets_from_specs.

If we think it's desirable, maybe we should build another peer utility to external_assets_from_specs which produces a single AssetsDefinition since we may want this pattern elsewhere

[benpankow]

See above comment but looks good

[sryza]

external_assets_from_spec does more than just create a multi-asset with a non-working compute function. It also adds some metadata to indicate that the asset is neither materializable nor observable, which the UI respects in certain situations. I think we should add something like external_assets_from_specs if we think it's important to create a single AssetsDefinition with multiple specs.

[rexledesma]

It also adds some metadata to indicate that the asset is neither materializable nor observable, which the UI respects in certain situations.

That metadata is added in this PR.

Is the concern that there should only be one function that sets this metadata in the first place (e.g. external_assets_from_specs)? If that's the case, I can add some utility function that adds this metadata to a set of asset specs.

I think we should add something like external_assets_from_specs if we think it's important to create a single AssetsDefinition with multiple specs.

I want to avoid the situation where we have something like:

• external_asset_from_spec(spec: AssetSpec) -> AssetsDefinition
• external_assets_from_specs(specs: Sequence[AssetSpec]) -> List[AssetsDefinition]
• [Proposed?] external_asset_from_specs(specs: Sequence[AssetSpec]) -> AssetsDefinition

which is pretty confusing.

[sryza]

@smackesey @schrockn – do either of you have thoughts here?

[schrockn]

We should change external_assets_from_specs or have a supported API for producing a single AssetsDefinition. It originally did return a single AssetsDefinition but I got pushback on that and just relented, which was a mistake.

[sryza]

It originally did return a single AssetsDefinition but I got pushback on that and just relented, which was a mistake.

I dug up a comment where I was one of the people pushing back: #16754 (comment).

My thinking has definitely moved in a different direction since then. E.g. this discussion is basically advocating for the exact opposite: instead of expecting an AssetsDefinition to be the smallest indivisible unit, make it possible to mash AssetsDefinition (or its successor class) into larger objects with N asset specs and N compute functions.