Skip to content
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

[dagster-dbt][refactor] Clean up multi-asset creation #26614

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OwenKephart
Copy link
Contributor

@OwenKephart OwenKephart commented Dec 19, 2024

Summary & Motivation

We have a bunch of complex logic going on here, purely because we are not taking advantage of the features around passing specs into @multi_asset

Crucially, before this change, we were manually creating internal_asset_deps, AssetIns, and AssetOuts independently, alongside juggling a bunch of different dictionaries to keep track of it all.

Now, I've created a single get_asset_spec method. Once we're ready, this can essentially be copy-pasted into the DagsterDbtTranslator class once we're ready, but for now this is purely-internal construct.

The only change in the constructed object is that we no longer have control over the generated output names for each key, and instead they are automatically generated using AssetKey.to_python_identifier. This is the cause of the updated tests.

I judge this is totally fine -- the output names are not actually user-facing in this context, it's just framework code that needs to handle this mapping (and this framework code actually gets a bit nicer as well)

How I Tested These Changes

Changelog

NOCHANGELOG

@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_use_assetout.from_spec_to_create_assetouts_for_dbt_assets_decorator branch from 69492b9 to 802dc85 Compare December 19, 2024 21:34
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation branch from 1247848 to 5a81aac Compare December 19, 2024 21:34
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_use_assetout.from_spec_to_create_assetouts_for_dbt_assets_decorator branch from 802dc85 to 03c57d7 Compare December 20, 2024 14:09
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_use_assetout.from_spec_to_create_assetouts_for_dbt_assets_decorator branch from 03c57d7 to 7c0dd74 Compare December 20, 2024 15:53
@OwenKephart OwenKephart changed the base branch from 12-19-_dagster-dbt_refactor_use_assetout.from_spec_to_create_assetouts_for_dbt_assets_decorator to graphite-base/26614 December 20, 2024 16:26
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation branch from 5a81aac to 045075a Compare December 21, 2024 20:51
@OwenKephart OwenKephart changed the base branch from graphite-base/26614 to master December 21, 2024 20:52
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation branch from 045075a to d07cd21 Compare December 21, 2024 21:40
@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation branch from d07cd21 to d74987c Compare December 21, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant