dagster icon indicating copy to clipboard operation
dagster copied to clipboard

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

Open sryza opened this issue 9 months ago • 4 comments

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

sryza avatar May 17 '24 15:05 sryza