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] Spec-ify dbt cloud integration #26662

Merged

Conversation

OwenKephart
Copy link
Contributor

@OwenKephart OwenKephart commented Dec 21, 2024

Summary & Motivation

Updates the dbt cloud integration to be more spec-native, taking advantage of existing utilities for generating specs from a dbt project.

This results in the ability to delete a large amount of code.

How I Tested These Changes

Changelog

NOCHANGELOG

@OwenKephart OwenKephart marked this pull request as draft December 21, 2024 21:20
@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-21-_dagster-dbt_spec-ify_dbt_cloud_integration branch from 4599035 to cd1dfdc 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
@OwenKephart OwenKephart force-pushed the 12-21-_dagster-dbt_spec-ify_dbt_cloud_integration branch 2 times, most recently from 5f32d81 to c10af5c Compare December 21, 2024 22:09
@OwenKephart OwenKephart marked this pull request as ready for review December 23, 2024 15:08
@@ -890,137 +881,6 @@ def _validate_asset_keys(
)


def get_asset_deps(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so wonderful

deps=dbt_dependencies,
# TODO: In the future, allow the IO manager to be specified.
io_manager_key=None,
# generate specs for each executed node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filler comment

manifest=None,
select=" ".join(
f"fqn:{'.'.join(dbt_nodes[unique_id]['fqn'])}" for unique_id in executed_unique_ids
),
)

return AssetsDefinitionCacheableData(
# TODO: In the future, we should allow additional upstream assets to be specified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we're here can we get rid of this / make a linear ticket (but I think this is possible via map asset specs now, so not really necessary

keys_by_output_name={
output_name: asset_key for asset_key, (output_name, _) in asset_outs.items()
},
keys_by_output_name={spec.key.to_python_identifier(): spec.key for spec in specs},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somewhat surprised we don't also have to include checks here, but it doesn't look like we did before either

Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few comments but generally awesome

@OwenKephart OwenKephart force-pushed the 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation branch from d74987c to 35c9420 Compare December 23, 2024 16:58
@OwenKephart OwenKephart force-pushed the 12-21-_dagster-dbt_spec-ify_dbt_cloud_integration branch from c10af5c to be8c8bb Compare December 23, 2024 16:58
Copy link
Contributor Author

OwenKephart commented Dec 30, 2024

Merge activity

  • Dec 30, 9:41 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 30, 9:46 AM EST: Graphite rebased this pull request as part of a merge.
  • Dec 30, 9:47 AM EST: A user merged this pull request with Graphite.

@OwenKephart OwenKephart changed the base branch from 12-19-_dagster-dbt_refactor_clean_up_multi-asset_creation to graphite-base/26662 December 30, 2024 14:42
@OwenKephart OwenKephart changed the base branch from graphite-base/26662 to master December 30, 2024 14:43
@OwenKephart OwenKephart force-pushed the 12-21-_dagster-dbt_spec-ify_dbt_cloud_integration branch from be8c8bb to ca1bbf9 Compare December 30, 2024 14:45
@OwenKephart OwenKephart merged commit b9dd154 into master Dec 30, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 12-21-_dagster-dbt_spec-ify_dbt_cloud_integration branch December 30, 2024 14:47
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.

2 participants