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

Extend the same keyword arguments callable support in PythonOperator to some other sensors/operators #11922

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

yuqian90
Copy link
Contributor

@yuqian90 yuqian90 commented Oct 28, 2020

  • This PR Standardises the callable signatures in PythonOperator, PythonSensor, ExternalTaskSensor, SimpleHttpOperator and HttpSensor.
  • The callable facilities in PythonOperator have been refactored into airflow.utils.helper.make_kwargs_callable. And it's used in those other places to make them work the same way.

PythonOperator and PythonSensor already accept python_callable in this form, i.e. the callable can request any number of named arguments in context:

def python_callable(execution_date, ds_nodash):
   ...

However, ExternalTaskSensor and some other sensors/operator accepts only callables in this form:

def execution_date_fn(execution_date):
   ...

A more recent change #8702 added support for this form as well:

def execution_date_fn(execution_date, context):
   ...

Unfortunately, #8702 introduced an unpleasant side-effect. Because it enforces the number of arguments is either 1 or 2, it stops people from using common techniques such as loop variable capturing. E.g. this comment illustrate a problem: https://github.com/apache/airflow/pull/8702/files#r497897460

This PR addresses these problems by making execution_date_fn work the same way as python_callable. execution_date_fn is now much more flexible. It can accept any variable number of keyword arguments from context. The change is backward compatible. Existing usage continues to work.

E.g. the following callables are all legitimate execution_date_fn after this PR:

            def execution_date_fn(dt):
                ...

            def execution_date_fn(execution_date):
                ...

            def execution_date_fn(execution_date, context):
                ...

            def execution_date_fn(execution_date, ds_nodash):
                ...

            def execution_date_fn(execution_date, ds_nodash, dag):
                ...

@yuqian90 yuqian90 changed the title Extend the same keyword arguments callable support in PythonOperator to ExternalTaskSensor Extend the same keyword arguments callable support in PythonOperator to ExternalTaskSensor Oct 28, 2020
@yuqian90
Copy link
Contributor Author

@Fokko @ashb @BasPH @mik-laj hope you are interested since you worked on/reviewed #5990.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@yuqian90 yuqian90 changed the title Extend the same keyword arguments callable support in PythonOperator to ExternalTaskSensor Extend the same keyword arguments callable support in PythonOperator to some other sensors/operators Oct 29, 2020
@mik-laj
Copy link
Member

mik-laj commented Oct 30, 2020

It is a backward compatible change?

@yuqian90
Copy link
Contributor Author

It is a backward compatible change?

It's backward compatible with all existing usage except this one:

def execution_date_fn(dt, ctx)

I.e. if context from #8702 is needed, the argument must be named context and not ctx because it's now a keyword argument. This will continue to work:

def execution_date_fn(dt, context)

I'll add this to UPDATING.md.

@yuqian90 yuqian90 force-pushed the remove_kwargs branch 2 times, most recently from 7902154 to 7b6ac9c Compare November 1, 2020 09:08
@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

@yuqian90
Copy link
Contributor Author

yuqian90 commented Nov 4, 2020

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

Thanks. I've rebased.

airflow/utils/helpers.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 9, 2020
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@ashb ashb added this to the Airflow 2.0.0-beta1 milestone Nov 9, 2020
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

yuqian90 and others added 2 commits November 9, 2020 19:41
- Use determine_kwargs in python.py
- Use make_kwargs_callable in external_task_sensor.py
@ashb
Copy link
Member

ashb commented Nov 9, 2020

I'm tempted to merge this anyway -- most tests are passing.

@kaxil kaxil merged commit badd890 into apache:master Nov 9, 2020
@yuqian90
Copy link
Contributor Author

yuqian90 commented Nov 9, 2020

Thanks @ashb and @kaxil !

potiuk added a commit to potiuk/airflow that referenced this pull request Apr 6, 2021
Fixes failing import problem of the http backport
provider with Airflow 1.10.* series.

A problem was introduced in apache#11922 which cause the http provider
to stop working (local import was not caught at the review time
and as local import it has not been caught by the test harness).

Since the http provider is defunct and is very popular, we
decided to release an out-of-band release of the http provider
even if backport providers reached end-of-life.

This PR copies implementation of make_kwargs_callable into http
backport provider to deal with the incompatibility.

Fixes: apache#15198
potiuk added a commit that referenced this pull request Apr 6, 2021
Fixes failing import problem of the http backport
provider with Airflow 1.10.* series.

A problem was introduced in #11922 which cause the http provider
to stop working (local import was not caught at the review time
and as local import it has not been caught by the test harness).

Since the http provider is defunct and is very popular, we
decided to release an out-of-band release of the http provider
even if backport providers reached end-of-life.

This PR copies implementation of make_kwargs_callable into http
backport provider to deal with the incompatibility.

Fixes: #15198
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants