[spec-ify-assets-def] replace dicts inside AssetsDefinition with dict of AssetSpecs

[sryza]

Summary & Motivation

This PR is the star of the "[spec-ify-assets-def]" stack.

It replaces all of these properties on AssetsDefinition:

    _asset_deps: Mapping[AssetKey, AbstractSet[AssetKey]]
    _group_names_by_key: Mapping[AssetKey, str]
    _metadata_by_key: Mapping[AssetKey, ArbitraryMetadataMapping]
    _tags_by_key: Mapping[AssetKey, Mapping[str, str]]
    _freshness_policies_by_key: Mapping[AssetKey, FreshnessPolicy]
    _auto_materialize_policies_by_key: Mapping[AssetKey, AutoMaterializePolicy]
    _code_versions_by_key: Mapping[AssetKey, Optional[str]]
    _descriptions_by_key: Mapping[AssetKey, str]
    _owners_by_key: Mapping[AssetKey, Sequence[str]]

with just:

    _specs_by_key: Mapping[AssetKey, AssetSpec]

It should have no effect on behavior or public APIs.

A couple things to think about here:

• Perf. Every time we copy an AssetsDefinition, we need to go back and forth between the two representations. I have followup PRs that make this no longer the case, but that are more invasive.
• AssetSpec vs. AssetDefinition vs. AssetNode. At times, we've discussed the possibility that AssetSpec should not be the internal object of record that we use to describe assets. This change does not lock us into AssetSpec being the internal object of record. By consolidating all these dicts into one place, this change should make it easier to swap it out for something else in the future if we choose to do so.

How I Tested These Changes

[smackesey]

I heartily approve this direction. Some inline comments. I also wonder whether we can get rid of a lot of the non-public (*_by_key) properties in a follow-up and instead access the specs with a get() method. That's the pattern that AssetGraph and AssetLayer now use.

[smackesey]

I didn't look at this super-carefully but sort of seems like this logic should be contained in specs_from_asset_key_params or another function rather than inlined into constructor.

[sryza]

specs_from_asset_key_params is narrowly focused on converting from the many-dicts representation to the specs representation.

The reason that encapsulating that is important is that, in followup PRs, we'll allow passing in specs to the AssetsDefinition constructor directly. specs_from_asset_key_params will only be needed when many-dicts are passed in, but the validation logic will need to run either way.

[smackesey]

Aren't a lot of these by_key dicts already properties you could just read?

Also, it's unfortunate that we have to round-trip through this *_by_key pattern when copying. Great that you have follow-ups that address that.

[sryza]

Aren't a lot of these by_key dicts already properties you could just read?

Yes, but each of those properties requires iterating over the specs_by_key dict. So using them would require iterating over that dict once per property, instead of just once.

And I'd like to eventually eliminate those properties (which are private) and expect people to go through the specs.

[smackesey]

Also, why AssetSpec instead of AssetNode (I understand we're not locked in though)?

[sryza]

Also, why AssetSpec instead of AssetNode

The main reasons:

• AssetSpec is already involved in AssetsDefinition construction. I.e. it's already an argument to @multi_asset, so seems like it's not a stretch to make it an argument to the AssetsDefinition constructor (even though that's not done in this PR).
• AssetSpec is public. This means we can make AssetsDefinition.specs_by_key a public method (even though that's not done in this PR)..

[smackesey]

I think we will want to transition to AssetNode later (and convert specs to nodes in the constructor) so that our internals are maximally decoupled from public AssetSpec but I think AssetSpec is fine for now. ✅

[schrockn]

You love to see it