-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[spec-ify-assets-def] replace dicts inside AssetsDefinition with dict of AssetSpecs #21937
Conversation
7895327
to
377d349
Compare
377d349
to
362059e
Compare
362059e
to
8a48268
Compare
8a48268
to
78661d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
) | ||
) | ||
|
||
self._specs_by_key = {spec.key: spec for spec in normalized_specs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
is_subset=self._is_subset, | ||
owners_by_key=owners_by_key, | ||
tags_by_key=tags_by_key, | ||
is_subset=self.is_subset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also, why |
The main reasons:
|
I think we will want to transition to |
You love to see it |
… of AssetSpecs branch-name: specify-internal
Summary & Motivation
This PR is the star of the "[spec-ify-assets-def]" stack.
It replaces all of these properties on
AssetsDefinition
:with just:
It should have no effect on behavior or public APIs.
A couple things to think about here:
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 thatAssetSpec
should not be the internal object of record that we use to describe assets. This change does not lock us intoAssetSpec
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