You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/12 13:34:45 UTC
[GitHub] [airflow] blcksrx opened a new pull request, #25000: set default task group in dag.add_task method
blcksrx opened a new pull request, #25000:
URL: https://github.com/apache/airflow/pull/25000
related: #24996
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25000:
URL: https://github.com/apache/airflow/pull/25000#issuecomment-1181935067
Tests need fixing and likely new tests for this specific case should be added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25000:
URL: https://github.com/apache/airflow/pull/25000#issuecomment-1195213683
This has a weird side effect that the task is identified in the DAG under a different `task_id` than the `task_id` attribute set on that task object. I don’t think I am particularly fond with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blcksrx commented on pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
blcksrx commented on PR #25000:
URL: https://github.com/apache/airflow/pull/25000#issuecomment-1195312183
Yeah Indeed, Im working on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25000:
URL: https://github.com/apache/airflow/pull/25000
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25000:
URL: https://github.com/apache/airflow/pull/25000#discussion_r929712844
##########
tests/models/test_dag.py:
##########
@@ -1403,6 +1404,18 @@ def test_create_dagrun_job_id_is_set(self):
)
assert dr.creating_job_id == job_id
+ def test_dag_add_task_sets_default_task_group(self):
+ dag = DAG(dag_id="test_dag_add_task_sets_default_task_group", start_date=DEFAULT_DATE)
+ task_without_task_group = EmptyOperator(task_id="task_without_group_id")
+ default_task_group = TaskGroupContext.get_current_task_group(dag)
+ dag.add_task(task_without_task_group)
+ assert default_task_group.get_child_by_label("task_without_group_id") is not None
Review Comment:
Instead of `is not None` is it better to explicitly compare it to `dag.get_task("task_without_group_id")`?
##########
tests/models/test_dag.py:
##########
@@ -1403,6 +1404,18 @@ def test_create_dagrun_job_id_is_set(self):
)
assert dr.creating_job_id == job_id
+ def test_dag_add_task_sets_default_task_group(self):
+ dag = DAG(dag_id="test_dag_add_task_sets_default_task_group", start_date=DEFAULT_DATE)
+ task_without_task_group = EmptyOperator(task_id="task_without_group_id")
+ default_task_group = TaskGroupContext.get_current_task_group(dag)
+ dag.add_task(task_without_task_group)
+ assert default_task_group.get_child_by_label("task_without_group_id") is not None
Review Comment:
Instead of `is not None` is it better to explicitly compare it to `dag.get_task(...)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25000:
URL: https://github.com/apache/airflow/pull/25000#discussion_r930607809
##########
airflow/models/taskmixin.py:
##########
@@ -210,9 +209,7 @@ def add_only_new(obj, item_set: Set[str], item: str) -> None:
for task in task_list:
if dag and not task.has_dag():
# If the other task does not yet have a dag, add it to the same dag as this task and
- # put it in the dag's root TaskGroup.
Review Comment:
Accidental? The comment now ends with a dangling `and`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25000:
URL: https://github.com/apache/airflow/pull/25000#issuecomment-1196253281
When you fix the `task_id` issue, please also add a line in the test to ensure `task_with_task_group.task_id` is correctly prefixed by the task group ID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] blcksrx commented on pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
blcksrx commented on PR #25000:
URL: https://github.com/apache/airflow/pull/25000#issuecomment-1200826844
> When you fix the `task_id` issue, please also add a line in the test to ensure `task_with_task_group.task_id` is correctly prefixed by the task group ID.
@uranusjr done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25000: set default task group in dag.add_task method
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25000:
URL: https://github.com/apache/airflow/pull/25000#discussion_r929712844
##########
tests/models/test_dag.py:
##########
@@ -1403,6 +1404,18 @@ def test_create_dagrun_job_id_is_set(self):
)
assert dr.creating_job_id == job_id
+ def test_dag_add_task_sets_default_task_group(self):
+ dag = DAG(dag_id="test_dag_add_task_sets_default_task_group", start_date=DEFAULT_DATE)
+ task_without_task_group = EmptyOperator(task_id="task_without_group_id")
+ default_task_group = TaskGroupContext.get_current_task_group(dag)
+ dag.add_task(task_without_task_group)
+ assert default_task_group.get_child_by_label("task_without_group_id") is not None
Review Comment:
Instead of `is not None` is it better to explicitly compare it to `task_without_task_group`? (Same question for the variant below)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org