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

Remove dag.run() method #42761

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Oct 5, 2024

This method uses Backfill internally. Before we can remove BackfillJobRunner, we need to remove DAG.run. But before we can remove DAG.run, we need to update some old tests that use it. So this is the first step towards removing BackflilJobRunner.

There were some very old tests that came from airflow github issue 1225. These appeared to test the scheduler but really they tested the backfill job runner. Just to be cautious, I kept most of them rather than remove (which probably would have been fine since they essentially tested code that we'll be removing). As appropriate I either changed them to run on dag.test or scheduler. The ones dealing with ignore first depends on past will have to be added back when that functionality is implemented in new backfill.

@boring-cyborg boring-cyborg bot added area:dev-tools area:Executors-core LocalExecutor & SequentialExecutor area:providers kind:documentation provider:google Google (including GCP) related issues labels Oct 5, 2024
@potiuk
Copy link
Member

potiuk commented Oct 5, 2024

🧛

@dstandish dstandish force-pushed the aip-78-remove-DAG-run-method branch 2 times, most recently from e07e652 to 0f1eb90 Compare October 8, 2024 17:21
@dstandish dstandish added the full tests needed We need to run full set of tests for this PR to merge label Oct 8, 2024
@dstandish dstandish force-pushed the aip-78-remove-DAG-run-method branch 3 times, most recently from 01a235e to d74c9ea Compare October 9, 2024 05:28
@dstandish dstandish removed the full tests needed We need to run full set of tests for this PR to merge label Oct 9, 2024
@dstandish dstandish marked this pull request as ready for review October 9, 2024 05:29
@dstandish dstandish force-pushed the aip-78-remove-DAG-run-method branch 4 times, most recently from 2e158aa to c0d2b0d Compare October 9, 2024 22:27
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Few nits/small changes.

And we should fix those broken google system test dags, or perhaps since they clearly couldn't have been being run remove them entirely?

@dstandish dstandish requested a review from ashb October 10, 2024 14:17
@dstandish dstandish force-pushed the aip-78-remove-DAG-run-method branch 2 times, most recently from a727814 to 20c6a70 Compare October 10, 2024 15:38
@dstandish dstandish merged commit 70b8e50 into apache:main Oct 10, 2024
76 checks passed
@dstandish dstandish deleted the aip-78-remove-DAG-run-method branch October 10, 2024 21:39
kunaljubce pushed a commit to kunaljubce/airflow that referenced this pull request Oct 13, 2024
This method uses Backfill internally. Before we can remove BackfillJobRunner, we need to remove DAG.run. But before we can remove DAG.run, we need to update some old tests that use it. So this is the first step towards removing BackflilJobRunner.

There were some very old tests that came from airflow github issue 1225. These appeared to test the scheduler but really they tested the backfill job runner. Just to be cautious, I kept most of them rather than remove (which probably would have been fine since they essentially tested code that we'll be removing). As appropriate I either changed them to run on dag.test or scheduler. The ones dealing with ignore first depends on past will have to be added back when that functionality is implemented in new backfill.
pavansharma36 pushed a commit to pavansharma36/airflow that referenced this pull request Oct 14, 2024
This method uses Backfill internally. Before we can remove BackfillJobRunner, we need to remove DAG.run. But before we can remove DAG.run, we need to update some old tests that use it. So this is the first step towards removing BackflilJobRunner.

There were some very old tests that came from airflow github issue 1225. These appeared to test the scheduler but really they tested the backfill job runner. Just to be cautious, I kept most of them rather than remove (which probably would have been fine since they essentially tested code that we'll be removing). As appropriate I either changed them to run on dag.test or scheduler. The ones dealing with ignore first depends on past will have to be added back when that functionality is implemented in new backfill.
R7L208 pushed a commit to R7L208/airflow that referenced this pull request Oct 17, 2024
This method uses Backfill internally. Before we can remove BackfillJobRunner, we need to remove DAG.run. But before we can remove DAG.run, we need to update some old tests that use it. So this is the first step towards removing BackflilJobRunner.

There were some very old tests that came from airflow github issue 1225. These appeared to test the scheduler but really they tested the backfill job runner. Just to be cautious, I kept most of them rather than remove (which probably would have been fine since they essentially tested code that we'll be removing). As appropriate I either changed them to run on dag.test or scheduler. The ones dealing with ignore first depends on past will have to be added back when that functionality is implemented in new backfill.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:Executors-core LocalExecutor & SequentialExecutor area:providers kind:documentation provider:google Google (including GCP) related issues
Projects
Development

Successfully merging this pull request may close these issues.

4 participants