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/08/08 12:50:11 UTC
[GitHub] [airflow] josh-fell opened a new pull request, #25604: Add `output` property to common operator implementation
josh-fell opened a new pull request, #25604:
URL: https://github.com/apache/airflow/pull/25604
Currently the `output` property of operators is only available to unmapped "classic" operators. There can be use cases in which `XComs` from a set of mapped tasks need to be consumed by a downstream task. Of course, accessing these mapped-task `XComs` can be done with the typical Jinja template, but having the convenience of using the `output` property would be a "quality of life" improvement for DAG authors (especially since task dependencies would then be automatically implemented too).
Rather than adding the property to `MappedOperator` and have this property definition exist in two locations (along with `BaseOperator`), I opted to add this to `AbstractOperator` given all tasks have an output even if that output is empty and both `BaseOperator` and `MappedOperator` share the `AbstractOperator` implementation.
TODO:
- Add docs demonstrating the use of this property with mapped tasks.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] josh-fell commented on a diff in pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25604:
URL: https://github.com/apache/airflow/pull/25604#discussion_r953106261
##########
airflow/providers/amazon/aws/example_dags/example_ecs.py:
##########
@@ -99,18 +100,20 @@
)
# [END howto_operator_ecs_register_task_definition]
+ registered_task_definition = cast(str, register_task.output)
Review Comment:
Great! I was going to ask if there was a better way than doing all of these `cast()` calls. Had to be something we could do with mypy specifically.
--
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] josh-fell commented on a diff in pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25604:
URL: https://github.com/apache/airflow/pull/25604#discussion_r953116395
##########
airflow/example_dags/example_xcom.py:
##########
@@ -79,8 +79,8 @@ def pull_value_from_bash_push(ti=None):
bash_pull = BashOperator(
task_id='bash_pull',
bash_command='echo "bash pull demo" && '
- f'echo "The xcom pushed manually is {bash_push.output["manually_pushed_value"]}" && '
- f'echo "The returned_value xcom is {bash_push.output}" && '
+ f'echo "The xcom pushed manually is {XComArg(bash_push, key="manually_pushed_value")}" && '
+ f'echo "The returned_value xcom is {XComArg(bash_push)}" && '
Review Comment:
Although maybe as long as each file is consistent that's fine. `.output` is still a valid and useful API. I don't think we want to rip it out of all of the examples.
--
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 #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25604:
URL: https://github.com/apache/airflow/pull/25604#discussion_r953103474
##########
airflow/example_dags/example_xcom.py:
##########
@@ -79,8 +79,8 @@ def pull_value_from_bash_push(ti=None):
bash_pull = BashOperator(
task_id='bash_pull',
bash_command='echo "bash pull demo" && '
- f'echo "The xcom pushed manually is {bash_push.output["manually_pushed_value"]}" && '
- f'echo "The returned_value xcom is {bash_push.output}" && '
+ f'echo "The xcom pushed manually is {XComArg(bash_push, key="manually_pushed_value")}" && '
+ f'echo "The returned_value xcom is {XComArg(bash_push)}" && '
Review Comment:
The latter one can still use `output` although using `XComArg` for both is probably more consistent…?
--
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] josh-fell commented on a diff in pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25604:
URL: https://github.com/apache/airflow/pull/25604#discussion_r953105640
##########
airflow/example_dags/example_xcom.py:
##########
@@ -79,8 +79,8 @@ def pull_value_from_bash_push(ti=None):
bash_pull = BashOperator(
task_id='bash_pull',
bash_command='echo "bash pull demo" && '
- f'echo "The xcom pushed manually is {bash_push.output["manually_pushed_value"]}" && '
- f'echo "The returned_value xcom is {bash_push.output}" && '
+ f'echo "The xcom pushed manually is {XComArg(bash_push, key="manually_pushed_value")}" && '
+ f'echo "The returned_value xcom is {XComArg(bash_push)}" && '
Review Comment:
Yeah, it might be better to be consistent. There are some other places I need to clean that up to use `XComArg()` throughout rather than `.output` for now. Is that cool?
--
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] josh-fell commented on pull request #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1215607518
Side note, the example DAGs were all updated as best as possible to follow the new TaskFlow syntax (see #10285). I have no problem updating these again.
--
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] josh-fell commented on pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1228661090
🤞 on CI. I think I finally got everything and new stuff.
--
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 #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1216149195
> WDYT about allowing `operator.output(key=...)`?
Doesn’t this conflict with using `op.output` as a property though? It’s possible but quite difficult to implement `output` as both an XComArg and callable. Maybe `op.get_xcom(key=...)` (or something like that) would be easier.
The general direction feels like a good idea to me though. I also plan to add some mechanism in #25661 to avoid exposing the bracket syntax:
1. Keep `XComArg(...)` and the return value of `@task` work as-is (so any existing code depending on the bracket syntax continues to work)
2. Introduce a new XComArg class that _does not_ provide the bracket syntax, and use that in `output` and `get_xcom()` etc.
Does this sound like a good plan?
--
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 #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25604:
URL: https://github.com/apache/airflow/pull/25604#discussion_r953105117
##########
airflow/providers/amazon/aws/example_dags/example_ecs.py:
##########
@@ -99,18 +100,20 @@
)
# [END howto_operator_ecs_register_task_definition]
+ registered_task_definition = cast(str, register_task.output)
Review Comment:
It’s probably a good idea to add a Mapy plugin for this… I’ll look into it after this is merged.
--
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 #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1211566287
Hmm, the static check failures raise a point for discussion. XComArg has an (unfortunate) API design `xcom_arg[key]` that can be used to pull an XCom under a different key, but I’m not sure I like it being used with `output`. `task.output["foo"]` feel to me more like accessing a dict, not pulling another XCom. The API design can be discussed/changed later, but here I think we should decide whether that is a usage we want to encourage or not (personally I’m against).
--
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] josh-fell commented on pull request #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1209634271
Yeah slowly trying to change the docs around -- at least to promote TaskFlow API instead of the classic operators. I logged #25319 to update code snippets throughout.
--
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] josh-fell commented on pull request #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1215593015
> Hmm, the static check failures raise a point for discussion. XComArg has an (unfortunate) API design `xcom_arg[key]` that can be used to pull an XCom under a different key, but I’m not sure I like it being used with `output`. `task.output["foo"]` feel to me more like accessing a dict, not pulling another XCom. The API design can be discussed/changed later, but here I think we should decide whether that is a usage we want to encourage or not (personally I’m against).
I have the same icky feeling with that current syntax as well. It's also not functionally equivalent to the classic `ti.xcom_pull(task_id=..., key=...)["some_item"]` which I [opened an issue about last year](https://github.com/apache/airflow/issues/16618). There was some discussion on what to change the API to, but there was also a little of "that ship has sailed" vibe too. Since this is coming up again, it's probably time to raise a discussion about it on the dev list.
In the meantime, WDYT about allowing `operator.output(key=...)`? It's functionally possible to pass in a `key` arg to `XComArg` already, the syntax isn't too bad, and has a less confusing API than the current one. I'm happy to work on this if it's happy middle ground.
--
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 #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1209211003
> TIL about `output`. Maybe we should promote it more instead of telling the user to import `XComArg` directly!
Yeah. Promoting more the TaskFlow and outputs is something we should really do.
I think we should start with changing our documentation with reversing the sequence and generally putting a different emphasis:
* "The right way of wriging tasks (Task Flow and Hooks)" and
* "The Old and legacy way of doing things (the operators).
--
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 merged pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #25604:
URL: https://github.com/apache/airflow/pull/25604
--
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] josh-fell commented on pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1223510489
> The general direction feels like a good idea to me though. I also plan to add some mechanism in #25661 to avoid exposing the bracket syntax:
>
> 1. Keep `XComArg(...)` and the return value of `@task` work as-is (so any existing code depending on the bracket syntax continues to work)
> 2. Introduce a new XComArg class that _does not_ provide the bracket syntax, and use that in `output` and `get_xcom()` etc.
>
> Does this sound like a good plan?
Sounds like a good one to me.
--
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 #25604: Add `output` property to common operator implementation
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1209116281
TIL about `output`. Maybe we should promote that more instead of telling the user to import `XComArg` directly!
--
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] josh-fell commented on pull request #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1228845425
Nice check to make sure providers would still be compatible with Airflow 2.2! Caught some import issues.
--
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 #25604: Add `output` property to MappedOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25604:
URL: https://github.com/apache/airflow/pull/25604#issuecomment-1228902254
> Nice check to make sure providers would still be compatible with Airflow 2.2! Caught some import issues.
:D :D :D -> sometimes those are REALLY useful (even if annoying).
--
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