-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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 how PythonSensor returns values from python_callable #28932
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
airflow/sensors/python.py
Outdated
return PokeReturnValue(bool(return_value)) | ||
return return_value if isinstance(return_value, PokeReturnValue) else PokeReturnValue(bool(return_value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need add tests for this scenario for avoid regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to check that the xcom_value
in the return from poke()
is populated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to resolve static checks, please follow this Guide
tests/sensors/test_python.py
Outdated
@@ -41,6 +42,10 @@ def test_python_sensor_raise(self): | |||
with pytest.raises(ZeroDivisionError): | |||
self.run_as_task(lambda: 1 / 0) | |||
|
|||
def test_python_sensor_xcom(self): | |||
poke_result = self.run_as_operator(fn=lambda: PokeReturnValue(True, "xcom")).poke({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.run_as_operator
test class method use for execute operator and return task for further validations, see usage in:
When you call poke
on already created and executed task you actually call it again without provide actual context I would not recommend to do this because it could lead to potential issues in the future if something changes.
Otherwise I suggest two options (It is not limited by this options):
- Run as a task and check resulting XCom value
- Create task without helpers and manually call
poke
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opted to run the poke
method without running the operator first. I wasn't sure how to properly get the context to check the xcom after the task is run.
Awesome work, congrats on your first merged pull request! |
…8932) * Update how PythonSensor returns values from python_callable * test poke returns the xcom value * update test to only run poke * reformat based on changes * use full if rather than ternary
* Update how PythonSensor returns values from python_callable * test poke returns the xcom value * update test to only run poke * reformat based on changes * use full if rather than ternary (cherry picked from commit b0f302e)
* Update how PythonSensor returns values from python_callable * test poke returns the xcom value * update test to only run poke * reformat based on changes * use full if rather than ternary (cherry picked from commit b0f302e)
* Update how PythonSensor returns values from python_callable * test poke returns the xcom value * update test to only run poke * reformat based on changes * use full if rather than ternary (cherry picked from commit b0f302e)
Have PythonSensor check if python_callable has returned a PokeReturnValue before coercing to boolean
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.