Skip to content

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Dec 29, 2021

Historically sensors do not support returning XCom unless you override the execute method.
With this change we can optionally return an XCom value by returning one along with True
in a tuple e.g. True, {"some": "information"}

A key motivator for this PR is this other PR to add @task.sensor decorator. With the present PR merged, we can make it so the sensor decorator can return a tuple True, {"some": "info"} to push xcom.

@dstandish dstandish changed the title Allow sensors to return XCom values Allow sensors to return XCom values using tuple Dec 29, 2021
@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

I think this is pretty dangerous if you consider that sensor implementing this might (and likely will) be run with Airflow pre 2.3.

It will work unexpectedly:

>>> not (True, "Xcom")
False
>>> not True
False
>>> not (False, "Xcom")
False
>>> not False
True

As a result, such sensor will always succed if run on Airlfow 2.3.

An interesting thing though, that it's not as bad. This will work fine if (and only if) we make sure that tuple is ONLY returned where the result is "True". And conicidentally - the "XCom" value is really ONLY needed when the returned value is True. There is really no point in storing XCom while the sensor is still running (see below).

So in order to keep compatibiliyt, allowed return values shoudl be one of

  • False
  • (True, "Xcom")

That would be difficult to enforce though. But....... This leads me to conclusion that we do not need Tuple AT ALL :).

If we assume that there is no need to store "", False, True or any other "un-truthy" value in XCom we could rather easily make backwards compatible implementation ny changing the contract of poke method to return "Truthy" value rather than Bool.

Then

  • in case True or False (Bool) is returned - do not store it in XCom (All Airlfow versions)
  • in case of "Truthy" non-Bool value is returned - store it in XCom (Airflow 2.3+ only)
  • also (if we really think it is needed) we could store "Falsey" (non Bool) value in Xcom but I think that is really not a good use case for sensors - usually the XCom should only be needed by some tasks that are triggered after sensor is complete).

This willl be backwards compatible - both existing Sensors will work in Airflow 2.3 and new sensors will work on Airlfow pre 2.3 (without storing XCom though).

The only potential incompatibility is that truthy (non-Bool) poke output will also be stored as Xcom (but it is mis-use of sensor - it only supposed to have Bool output pre Airflow 2.3). And it has much better interface than the Tuple IMHO.

@dstandish
Copy link
Contributor Author

Yeah I knew this approach is dangerous and that's why I also explored using a class here #20547.

Using that class is more heavyhanded but it allows us to precisely know the implementer's intention.

But your idea about "always returning the truthy value" as xcom is interesting.

The the only problems are, what if you want to return something that doesn't behave correctly with respect to truthyness.

In fact, pandas dataframes don't allow this!

import pandas as pd

df = pd.DataFrame([1,2,3])

if df:
    print('hi')

The other (smaller) concern would be unnecessary xcom pushing when the user does not need xcom.

@dstandish
Copy link
Contributor Author

BUT, it would be backwards compatible....

@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

BUT, it would be backwards compatible....

See #20547 -> I like this one much more as long as we add bool that will help those new Sensors to also work for Aiflow pre 2.3 properly.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

AAAAH NO. I am wrong. It's not going to work. When you run such new sensor on Airlfow 2.2, you will not have PokeReturnValue available! So it will not work.

@dstandish dstandish force-pushed the allow-sensor-to-return-xcom branch from 2e3343b to deb361f Compare January 28, 2022 18:10
Historically sensors do not support returning XCom unless you override the `execute` method.
With this change we can optionally return an XCom value by returning one along with `True`
in a tuple e.g. `True, {"some": "information"}`
@dstandish dstandish force-pushed the allow-sensor-to-return-xcom branch from deb361f to f233505 Compare January 28, 2022 20:48
@dstandish
Copy link
Contributor Author

this pr taken over by @mingshi-wang in #20656

@dstandish dstandish closed this Mar 1, 2022
@uranusjr uranusjr deleted the allow-sensor-to-return-xcom branch April 11, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants