Skip to content

Move DagBag to SDK and make it return SDK DAG objects - #53918

Merged
uranusjr merged 4 commits into
apache:mainfrom
astronomer:remove-dag-bag-in-execution
Aug 14, 2025
Merged

Move DagBag to SDK and make it return SDK DAG objects#53918
uranusjr merged 4 commits into
apache:mainfrom
astronomer:remove-dag-bag-in-execution

Conversation

@uranusjr

@uranusjr uranusjr commented Jul 30, 2025

Copy link
Copy Markdown
Member
  • Remove DagBag usages where read_dags_from_db=True
  • Remove db access from DagBag
    • Everything except auto expiring check in get_dag
  • Move DagBag to SDK (for use in dag parser and task runner) I give up

@uranusjr

Copy link
Copy Markdown
Member Author

The Databricks operator links are a mess… Not dealing with them right now, I believe they are already broken in 3.0 so we can just keep breaking it more.

@uranusjr
uranusjr force-pushed the remove-dag-bag-in-execution branch 13 times, most recently from c269b3e to e592a29 Compare August 1, 2025 10:23
@uranusjr uranusjr self-assigned this Aug 4, 2025
@uranusjr
uranusjr force-pushed the remove-dag-bag-in-execution branch 7 times, most recently from 5e1a912 to 3c85372 Compare August 5, 2025 11:33
@uranusjr
uranusjr force-pushed the remove-dag-bag-in-execution branch from c145194 to 5a9c7b7 Compare August 11, 2025 11:39
@uranusjr

Copy link
Copy Markdown
Member Author

Initially I planned to remove all db access, and move the DagBag class to SDK. However, after spending almost a week trying to debug some weird Kubernetes integration test failures, I give up. This no longer moves the class.

I removed all database access in DagBag except get_dag, where it checks whether a dag has expired and auto-refreshes it. I think we no longer needs this check since dagbag uses are ephemeral in Airflow now, but we will need to fix test cases to reflect that. This will be done in a later PR.

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! LGTM overall.

I removed all database access in DagBag except get_dag, where it checks whether a dag has expired and auto-refreshes it. I think we no longer needs this check since dagbag uses are ephemeral in Airflow now, but we will need to fix test cases to reflect that. This will be done in a later PR.

Does it mean if we want to remove db access for DagBag, we need to add another execution API route for checking whether the dag for given dag_id is expired?

The only db access in DagBag is orm_dag := DagModel.get_current, and orm_dag is used for checking is_expired.

@uranusjr

Copy link
Copy Markdown
Member Author

I think we ultimately should remove the functionality. Scheduler and API server (the long running processes) already do not use this class now. The worker and dag parsing process are short-lived and this refreshing logic should not kick in at all. The main issue is to fix the tests so they reflect this, and implement necessary refreshing when a test expect it to happen automatically in-process (but now only happens in the dag processor in production circumstances).

@uranusjr
uranusjr force-pushed the remove-dag-bag-in-execution branch 3 times, most recently from afd9cd4 to 28cdf2b Compare August 14, 2025 08:50
@uranusjr
uranusjr force-pushed the remove-dag-bag-in-execution branch from 28cdf2b to 0b2bb73 Compare August 14, 2025 10:34
@uranusjr
uranusjr merged commit 09efdff into apache:main Aug 14, 2025
105 checks passed
@uranusjr
uranusjr deleted the remove-dag-bag-in-execution branch August 14, 2025 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants