Emit error on duplicated DAG ID #15302
Conversation
uranusjr
requested review from
ashb,
kaxil,
turbaszek and
XD-DENG
as code owners
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
uranusjr
force-pushed
the
dag-name-duplication-error
branch
2 times, most recently
from
8edf964 to
0bf8363
Compare
|
I need to figure out why existing tests are failing first. |
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
|
Turns out the test suite actually contains quite several DAGs with duplicated ID 🙂 |
uranusjr
force-pushed
the
dag-name-duplication-error
branch
2 times, most recently
from
b4563de to
ecd0126
Compare
|
Whoops yeah -- lot more failing tests |
uranusjr
force-pushed
the
dag-name-duplication-error
branch
from
c38074d to
88607ac
Compare
|
Alright, I think I’ve managed to fix all the issues in the test suite, and added a test for the new exception. Not sure why some jobs are erroring out. |
| pool='test_backfill_pooled_task_pool', | ||
| owner='airflow', | ||
| start_date=datetime(2016, 2, 1), | ||
| ) |
There was a problem hiding this comment.
This file is a strict subset of test_issue_1225.py, and the two test_backfill_pooled_task_dag were entirely equivalent.
| @@ -27,5 +27,5 @@ | |||
| "start_date": DEFAULT_DATE, | |||
| } | |||
|
|
|||
| with DAG(dag_id="test_only_dummy_tasks", default_args=default_args, schedule_interval='@once') as dag: | |||
| with DAG(dag_id="test_dag_with_no_tags", default_args=default_args, schedule_interval='@once') as dag: | |||
There was a problem hiding this comment.
There is a test_only_dummy_tasks DAG in test_only_dummy_tasks.py. Probably a copypasta error. DAG ID is not relevant here; the test this DAG serves never checks it, only the DAG’s content.
| self.dagbag = models.DagBag( | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| cls.dagbag = models.DagBag( |
There was a problem hiding this comment.
Not a required change, but sharing the DAG bag across tests provides a slight bit of performance gain. Most test case classes actually uses this pattern, not sure why it’s not applied here.
| @@ -113,6 +113,7 @@ def _prepare_db(self): | |||
| dagbag = self.app.dag_bag # pylint: disable=no-member | |||
| dag = DAG(self.DAG_ID, start_date=timezone.parse(self.default_time)) | |||
| dag.sync_to_db() | |||
| dagbag.dags.pop(self.DAG_ID, None) | |||
There was a problem hiding this comment.
These dict.pop() calls reset app.bag_dag’s state across tests so it can be re-populated for new things to test. An alternative fix is to use a different DAG ID for each test, which generates a larger diff, and probably slightly slower (DAGs created here need to be sync-ed into the db, so more DAG IDs = more db inserts; I didn’t actually benchmark this though).
|
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
This will be shown in logs on initialization, and flashed in UI on later scheduled refreshes.
uranusjr
force-pushed
the
dag-name-duplication-error
branch
from
5cd7fe7 to
3d64ea7
Compare
uranusjr
force-pushed
the
dag-name-duplication-error
branch
from
3d64ea7 to
bd78739
Compare
|
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
|
Grrrrrr come ON. It’s midnight in the US and nobody is comitting things but me. |
This will be shown in logs on initialization, and flashed in UI on later
scheduled refreshes.
Close #15248.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.