[external-assets] Hoist resolution of input asset keys to RepositoryDataBuilder #20186
Conversation
smackesey
force-pushed
the
sean/external-assets-asset-graph-tweaks
branch
from
2c4a5a8 to
0625561
Compare
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
from
66ff903 to
00eb64c
Compare
smackesey
force-pushed
the
sean/external-assets-asset-graph-tweaks
branch
from
0625561 to
58dcd97
Compare
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
from
00eb64c to
2f03e42
Compare
smackesey
force-pushed
the
sean/external-assets-asset-graph-tweaks
branch
from
58dcd97 to
8beb90c
Compare
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
3 times, most recently
from
ae90738 to
5003390
Compare
|
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-8nzhr4fkf-elementl.vercel.app Direct link to changed pages: |
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
from
5003390 to
072ed7a
Compare
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
from
072ed7a to
c2031ac
Compare
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
3 times, most recently
from
9fa2ff0 to
42c75ff
Compare
There was a problem hiding this comment.
An excellent change.
It would be great if we could test that AssetsDefinition has fully resolved dependencies and check invariants at appropriate entry points to ensure that they don't sneak through.
Put another way, at what point is it illegal to pass in an AssetsDefinition to one of our APIs?
| def resolve_asset_job( | ||
| assets: Sequence[Union[AssetsDefinition, SourceAsset]], | ||
| *, | ||
| selection: Optional[CoercibleToAssetSelection] = None, | ||
| name: str = "asset_job", | ||
| resources: Mapping[str, object] = {}, | ||
| **kwargs: Any, | ||
| ) -> JobDefinition: | ||
| assets_defs = [a for a in assets if isinstance(a, AssetsDefinition)] | ||
| source_assets = [a for a in assets if isinstance(a, SourceAsset)] | ||
| selection = selection or assets_defs | ||
| return Definitions( | ||
| assets=[*assets_defs, *source_assets], | ||
| jobs=[define_asset_job(name, selection, **kwargs)], | ||
| resources=resources, | ||
| ).get_job_def(name) |
There was a problem hiding this comment.
why call this "resolve" asset job?
create_asset_job_from_assets? or something?
There was a problem hiding this comment.
Good point, it was bc we are getting the result of define_asset_job(...).resolve(...) but "resolution" here is kind of an implementation detail. Changed to create_test_asset_job.
smackesey
force-pushed
the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
from
42c75ff to
5401f3b
Compare
I think all of our public APIs go through a |
smackesey
deleted the
sean/external-assets-hoist-input-key-resolution-to-repo-builder
branch
…ataBuilder (#20186) ## Summary & Motivation We have some hairy logic that resolves "relative" asset keys associated with asset inputs. Currently this resolution is done in multiple internal places, which complicates internal codepaths dealing with assets. This PR hoists resolution to repository build time, in the same place that we convert source assets to assets def. This simplifies internal pathways. In order to support this change, I had to alter tests that were directly calling `UnresolvedAssetsJobDefinition.resolve` with an `AssetGraph` to instead go through repository construction. With the external asset conversion happening at the repo level, this is something that needed to be done anyway. ## How I Tested These Changes Existing test suite
Summary & Motivation
We have some hairy logic that resolves "relative" asset keys associated with asset inputs. Currently this resolution is done in multiple internal places, which complicates internal codepaths dealing with assets.
This PR hoists resolution to repository build time, in the same place that we convert source assets to assets def. This simplifies internal pathways.
In order to support this change, I had to alter tests that were directly calling
UnresolvedAssetsJobDefinition.resolvewith anAssetGraphto instead go through repository construction. With the external asset conversion happening at the repo level, this is something that needed to be done anyway.How I Tested These Changes
Existing test suite