[dagster-airlift][federation-apis] federation APIs accumulator #25857
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 12, 2024
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Nov 12, 2024
dpeng817
changed the title
dpeng817
changed the title
dpeng817
force-pushed
the
dpeng817/move_out_examples
branch
from
924767f to
9c3d3d3
Compare
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
from
3b23857 to
ca0b0d6
Compare
dpeng817
force-pushed
the
dpeng817/move_tutorial
branch
from
18e94a9 to
ba5b005
Compare
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
from
ca0b0d6 to
0f431c5
Compare
dpeng817
force-pushed
the
dpeng817/move_tutorial
branch
from
ba5b005 to
a3fe72d
Compare
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
2 times, most recently
from
6e8a029 to
252e061
Compare
dpeng817
changed the base branch from
dpeng817/move_tutorial
to
graphite-base/25857
dpeng817
force-pushed
the
graphite-base/25857
branch
from
a3fe72d to
dcdf600
Compare
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
from
252e061 to
66d03ae
Compare
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
2 times, most recently
from
790dad2 to
46df90c
Compare
benpankow
approved these changes
Nov 14, 2024
| @@ -36,7 +37,7 @@ | |||
| @dataclass | |||
| class AirflowInstanceDefsLoader(StateBackedDefinitionsLoader[SerializedAirflowDefinitionsData]): | |||
| airflow_instance: AirflowInstance | |||
| explicit_defs: Definitions | |||
| mapped_assets: Iterable[MappedAsset] | |||
There was a problem hiding this comment.
could we use Sequence instead of Iterable across this PR? doesn't seem like we have a strong reason to make it Iterable & Sequence is more consistent across other parts of codebase
| def enrich_airflow_mapped_assets( | ||
| mapped_assets: Iterable[MappedAsset], | ||
| airflow_instance: AirflowInstance, | ||
| ) -> Sequence[AssetsDefinition]: |
There was a problem hiding this comment.
do we know that the outputs here are AssetsDefinition, since the input could be specs?
There was a problem hiding this comment.
oh I see, _apply_airflow_data_to_specs forcibly definition-izes the specs, makes sense
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
from
46df90c to
e9d165b
Compare
dpeng817
changed the title
dpeng817
force-pushed
the
dpeng817/enrich_mapped_assets
branch
from
e9d165b to
5e8ff6d
Compare
|
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-r7d52yv93-elementl.vercel.app Direct link to changed pages: |
Graphite Automations"Add a 'docs-to-migrate' label to PRs with docs" took an action on this PR • (11/14/24)1 label was added to this PR based on Christopher DeCarolis's automation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is now being used to accumulate federation API commits before landing.
ORIGINAL DESCRIPTION:
Summary & Motivation
Right now, all airlift internals which operate over mapped airflow assets take in a fully-qualified Definitions object. This PR changes them to instead operate over an iterable of assets, and leaves the definitions munging to the callsites.
After making this change, it becomes easy to add asset-oriented airlift methods - and I demonstrate that with a standalone method to enrich airflow assets. We'll eventually be able to re-implement the full defs loader on top of this.
How I Tested These Changes
New test for the new method, existing test suite.
Changelog
NOCHANGELOG