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

Update base sensor operator to support XCOM return value #20656

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

mingshi-wang
Copy link
Contributor

@mingshi-wang mingshi-wang commented Jan 4, 2022

Inspired by the idea of #20547, this PR is to update the base sensor operator such that an optional XCom value can be returned and pushed when the operator is executed. Users only need to override the poke() method to return a PokeReturnValue object that encapsulates the XCOM value.

related: #20323

^ 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.

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Jan 4, 2022
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 5, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@josh-fell
Copy link
Contributor

CC @dstandish since it's in the same vein as the draft PR #20547. Maybe you've already given the thumbs-up somewhere else.

@mingshi-wang
Copy link
Contributor Author

CC @dstandish since it's in the same vein as the draft PR #20547. Maybe you've already given the thumbs-up somewhere else.
Hi @dstandish - can you please take a look at this PR? After this is merged, I will update #20530 to be based on this PR. Thanks!

@kurtqq
Copy link
Contributor

kurtqq commented Feb 20, 2022

that would be nice

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice one. Also it has the potential of being detected by our "min airflow vrsion" CI check (It would fail on import).

Anyone else? @dstandish ?

airflow/sensors/base.py Outdated Show resolved Hide resolved
@dstandish
Copy link
Contributor

Nice one. Also it has the potential of being detected by our "min airflow vrsion" CI check (It would fail on import).

Anyone else? @dstandish ?

It's good with me. I recall @potiuk that you were trying to find a way to make it possible to return a tuple. But I think this change should be compatible with that. What we could consider doing is, for 3.0, make it so a sensor must be true / false or PRV (i.e., no longer support "truthy" values) and then in 3.1 we could add support for tuple. WDYT?

@potiuk
Copy link
Member

potiuk commented Feb 27, 2022

It's good with me. I recall @potiuk that you were trying to find a way to make it possible to return a tuple. But I think this change should be compatible with that. What we could consider doing is, for 3.0, make it so a sensor must be true / false or PRV (i.e., no longer support "truthy" values) and then in 3.1 we could add support for tuple. WDYT?

I am good for it actually as is. I think we do not need to do anything more :) .
Comparing to our discussion I think the main difference is that the The PokeReturnValue with __bool__ will actually return true/false properly even for older versions of Airlfow (but it will not store xcom value).

And you should be able to start using it in our community providers even now. The way it should be done is something like:

try:
   from ... import PokeReturnValue
except ImportError:
   PokeReturnValue = None

...

    if PokeReturnValue is not None:
         return PokeReturnValue(is_done, xcom_value)
    else:
        if is_done:
             xcom_push(xcom_value) # this is not proper way needs to be done properly.
        return is_done

I think we will figure out the details with the first sensor which will use it (and it will be detected in CI by failing import for 2.1) and we can - then add some pre-commit checks and better information for the developers of providers (but this can be done later).

UPDATE: Actually we can make future providers also 100% compatible until they have > 2.3.0.

@potiuk
Copy link
Member

potiuk commented Feb 27, 2022

Which makes me think.. Well maybe actually we should add a documentation about it. @mingshi-wang - could you please add docs explaininig that this feature (pushing Xcom) is only available in 2.3.0 and add the example of how you can add provider compatible with both pre 2.3.0 and 2.1.0 ?

@potiuk
Copy link
Member

potiuk commented Feb 27, 2022

Actually I realized that you CAN make it working in community providers INCLUDING XCom Push - simply the provider sensors will have to push the xcom manually until we are > 2.3.0 (which will be ~ December or so.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Some doc update is needed as discussed.

@mingshi-wang
Copy link
Contributor Author

Some doc update is needed as discussed.

Sure - I will put in some doc.

@mingshi-wang
Copy link
Contributor Author

@potiuk - I have updated the doc. Could you take a look again? This will unblock me from finishing #20323. Thanks!

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

❤️ it

@potiuk potiuk merged commit cd35972 into apache:main Mar 21, 2022
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow full tests needed We need to run full set of tests for this PR to merge type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants