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 2021/01/19 12:58:47 UTC
[GitHub] [airflow] ashmeet13 opened a new pull request #13763: BaseBranchOperator will push to xcom by default.
ashmeet13 opened a new pull request #13763:
URL: https://github.com/apache/airflow/pull/13763
This change will allow BaseBranchOperator to xcom push the branch it chose to follow.
It will also add support to use the `do_xcom_push` parameter.
The change includes the returning the result of `choose_branch()` whenever `execute()` is called.
Currently the BaseBranchOperator neither pushes the chosen branch to xcom nor support's the `do_xcom_push` parameter.
Closes: #13704
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashmeet13 commented on a change in pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
ashmeet13 commented on a change in pull request #13763:
URL: https://github.com/apache/airflow/pull/13763#discussion_r560419328
##########
File path: airflow/operators/branch.py
##########
@@ -49,4 +49,6 @@ def choose_branch(self, context: Dict) -> Union[str, Iterable[str]]:
raise NotImplementedError
def execute(self, context: Dict):
- self.skip_all_except(context['ti'], self.choose_branch(context))
+ branches_to_execute = self.choose_branch(context)
+ self.skip_all_except(context['ti'], branches_to_execute)
+ return branches_to_execute
Review comment:
Not sure about this. I dont think it would always push.
I did run the test case that I added for checking xcom push ([Permalink to the test function](https://github.com/apache/airflow/pull/13763/files#diff-fadab52f3011a0eda8eb036bc5df54a5c31396bfc84e467ea0ccf7e01c16842eR173-R193)) with `do_xcom_push=False` and the value returned by - `ti.xcom_pull(task_ids='make_choice')` was `None`
Have I understood the issue wrong? I followed PythonBranchOperator as a guide to figure out what was needed to be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13763:
URL: https://github.com/apache/airflow/pull/13763#discussion_r561450346
##########
File path: airflow/operators/branch.py
##########
@@ -49,4 +49,6 @@ def choose_branch(self, context: Dict) -> Union[str, Iterable[str]]:
raise NotImplementedError
def execute(self, context: Dict):
- self.skip_all_except(context['ti'], self.choose_branch(context))
+ branches_to_execute = self.choose_branch(context)
+ self.skip_all_except(context['ti'], branches_to_execute)
+ return branches_to_execute
Review comment:
Oh yes,
https://github.com/apache/airflow/blob/3fd5ef355556cf0ad7896bb570bbe4b2eabbf46e/airflow/models/taskinstance.py#L1315-L1318
should take care of 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13763:
URL: https://github.com/apache/airflow/pull/13763#discussion_r561450346
##########
File path: airflow/operators/branch.py
##########
@@ -49,4 +49,6 @@ def choose_branch(self, context: Dict) -> Union[str, Iterable[str]]:
raise NotImplementedError
def execute(self, context: Dict):
- self.skip_all_except(context['ti'], self.choose_branch(context))
+ branches_to_execute = self.choose_branch(context)
+ self.skip_all_except(context['ti'], branches_to_execute)
+ return branches_to_execute
Review comment:
Oh yes,
https://github.com/apache/airflow/blob/3fd5ef355556cf0ad7896bb570bbe4b2eabbf46e/airflow/models/taskinstance.py#L1315-L1318
should take care of 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13763:
URL: https://github.com/apache/airflow/pull/13763
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13763:
URL: https://github.com/apache/airflow/pull/13763#discussion_r560321466
##########
File path: airflow/operators/branch.py
##########
@@ -49,4 +49,6 @@ def choose_branch(self, context: Dict) -> Union[str, Iterable[str]]:
raise NotImplementedError
def execute(self, context: Dict):
- self.skip_all_except(context['ti'], self.choose_branch(context))
+ branches_to_execute = self.choose_branch(context)
+ self.skip_all_except(context['ti'], branches_to_execute)
+ return branches_to_execute
Review comment:
This means it will always push right, even if do_xcom_push is False
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashmeet13 commented on a change in pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
ashmeet13 commented on a change in pull request #13763:
URL: https://github.com/apache/airflow/pull/13763#discussion_r560419328
##########
File path: airflow/operators/branch.py
##########
@@ -49,4 +49,6 @@ def choose_branch(self, context: Dict) -> Union[str, Iterable[str]]:
raise NotImplementedError
def execute(self, context: Dict):
- self.skip_all_except(context['ti'], self.choose_branch(context))
+ branches_to_execute = self.choose_branch(context)
+ self.skip_all_except(context['ti'], branches_to_execute)
+ return branches_to_execute
Review comment:
Not sure about this. I did run the test case that I added for checking xcom push ([Permalink to the test function](https://github.com/apache/airflow/pull/13763/files#diff-fadab52f3011a0eda8eb036bc5df54a5c31396bfc84e467ea0ccf7e01c16842eR173-R193)) with `do_xcom_push=False` and the value returned by - `ti.xcom_pull(task_ids='make_choice')` was `None`
Have I understood the issue wrong? I followed PythonBranchOperator as a guide to figure out what was needed to be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #13763: BaseBranchOperator will push to xcom by default.
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13763:
URL: https://github.com/apache/airflow/pull/13763
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org