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

Save tasks map as DbtToAirflowConverter property #1362

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

internetcoffeephone
Copy link

Description

Currently, if you want to modify a DAG after it has been rendered, you have to walk through the dag.dbt_graph, then puzzle the task IDs and task group IDs together by reverse-engineering your task rendering strategy.

This is cumbersome and error-prone, hence exposing the direct mapping as a DAG property makes a lot of sense. This allows you to walk the actual DBT graph, which makes e.g. adding Airflow sensors upstream of all source tasks much easier.

Breaking Change?

No

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

There is nothing new to test. The only addition is taking a value that was already being generated and exposing it as a class property.

Currently, if you want to modify a DAG after it has been rendered,
you have to walk through the dag.dbt_graph, then puzzle the task IDs
and task group IDs together by reverse-engineering your task rendering
strategy.

This is cumbersome and error-prone, hence exposing the direct mapping as
a DAG property makes a lot of sense. This allows you to walk the actual
DBT graph, which makes e.g. adding Airflow sensors upstream of all source
tasks much easier.
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Dec 3, 2024
@dosubot dosubot bot added the area:rendering Related to rendering, like Jinja, Airflow tasks, etc label Dec 3, 2024
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (cfb6bf2) to head (b256454).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1362   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files          67       67           
  Lines        4025     4026    +1     
=======================================
+ Hits         3865     3866    +1     
  Misses        160      160           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

HI @internetcoffeephone, thank you very much for your contribution.

The feature makes sense, and your implementation is very clean and straightforward.

I only have three requests:

  • Add a unitest test confirming this new behaviour;
  • Create an example DAG illustrating how you can leverage this feature in dev/dags; and
  • Either add docs or create an issue so we can address this (we also need to document that graph is exposed, so there is debt on our side).

If you can get these changes before 20 December, we'll release this feature as part of Cosmos 1.8.

@tatiana tatiana added this to the Cosmos 1.8.0 milestone Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rendering Related to rendering, like Jinja, Airflow tasks, etc size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants