[AIRFLOW-4456] Add sub-classable BaseBranchOperator #5231

chronitis · 2019-05-03T09:11:11Z

Jira

My PR addresses https://issues.apache.org/jira/browse/AIRFLOW-4456

Description

Here are some details about my PR, including screenshots of any UI changes:

This adds an abstract BranchOperator, which implementors can use to
create concrete operators for branching workflows fully encapsulated in
a class, rather than needing to pass an external callable (or
self-reference) to BranchPythonOperator. Also adds tests for same.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

tests/operators/test_branch_operator.py

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

ashb · 2019-05-03T10:39:53Z

If you're going to add this the existing BranchingPython operator should use it!

ashb · 2019-05-03T10:42:34Z

An alternative would be for you to subclass the existiing PythonBranchOperator like this in your code base:

class MyBranchOperator(PythonBranchOperator):
    def __int__(**kwargs):
        super().__init__(python_callable=self.choose_branch, **kwargs)

    def choose_branch(self):
        return 'task_a' # etc.

chronitis · 2019-05-03T11:14:49Z

I take the comment that this logic shouldn't be duplicated between this operator and BranchPythonOperator - I'll look at a better way of avoiding duplication.

The reason for not subclassing BranchPythonOperator was that creating a bound reference to the method in __init__ seems to cause issues with templating:

class MyBranch(BranchPythonOperator):
    template_fields = ('myvar', )
    def __init__(self, myvar, **kwargs):
       self.myvar = myvar
       super().__init__(python_callable=self.choose_branch)

    def choose_branch(self):
        # for reasons I'm not totally clear on, but I'm guessing the reference to `self` before `super()`
        # has returned creates a closure, any {{ templates }} in `self.myvar` here don't get evaluated
        return self.myvar

Less importantly, it seems a bit inelegant to need to pass a reference to the method like this.

ashb · 2019-05-03T11:22:41Z

myvar won't be templated until just before execute() is called - when were you looking at the value? In init or in choose_branch?

ashb · 2019-05-03T11:23:21Z

I take the comment that this logic shouldn't be duplicated between this operator and BranchPythonOperator - I'll look at a better way of avoiding duplication.

Yes, that was what I meant.

chronitis · 2019-05-03T11:48:54Z

myvar won't be templated until just before execute() is called - when were you looking at the value? In init or in choose_branch?

In choose_branch; if you inspect the operator via the web interface the rendered template, myvar comes out as expected (templates applied), but code in choose_branch (invoked from BranchPythonOperator.execute()) appears to see the the untemplated version.

chronitis · 2019-05-03T13:27:57Z

Here's a possible refactoring; moving the logic part into SkipMixin and building both BranchOperator and BranchPythonOperator off it. It's possible that BranchOperator is unnecessary now and users should just inherit SkipMixin directly in this logic.

(Tests pass except for the Kubernetes case which appears to have timed out).

codecov-io · 2019-05-07T06:26:02Z

Codecov Report

Merging #5231 into master will increase coverage by <.01%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #5231      +/-   ##
==========================================
+ Coverage   78.67%   78.67%   +<.01%     
==========================================
  Files         470      471       +1     
  Lines       30013    30023      +10     
==========================================
+ Hits        23613    23622       +9     
- Misses       6400     6401       +1

Impacted Files	Coverage Δ
airflow/models/skipmixin.py	`100% <100%> (ø)`	⬆️
airflow/operators/python_operator.py	`95.45% <100%> (-0.36%)`	⬇️
airflow/operators/branch_operator.py	`85.71% <85.71%> (ø)`
airflow/models/taskinstance.py	`92.42% <0%> (-0.18%)`	⬇️
airflow/models/dagbag.py	`92.27% <0%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1755c35...412b978. Read the comment docs.

chronitis · 2019-05-08T11:33:42Z

@ashb does this refactoring and documentation look reasonable?

potiuk · 2019-05-08T12:44:25Z

@ashb does this refactoring and documentation look reasonable?

Ash is on holidays for the next 3 weeks but I might take over from here. I will take a look later today/tomorrow ok?

chronitis · 2019-05-10T12:33:18Z

@potiuk fine with me. I'm grateful for whoever amongst the committers is willing to take a look.

ashb

Small naming change, but yes this looks good, thanks!

ashb · 2019-06-21T15:30:59Z

airflow/operators/branch_operator.py

Suggested change

class BranchOperator(BaseOperator, SkipMixin):

class BaseBranchOperator(BaseOperator, SkipMixin):

Just to make it a tiny bit clearer that this is intended to be sub-classed (i.e. not used in a DAG, similar to BaseOperator)

ashb

Improve doc linking to classes too.

The suggestions as they are will make the line-wrapping messy, so probably tidy that up too :)

docs/concepts.rst

ashb · 2019-06-21T15:52:29Z

@chronitis If you are able to make the small changes suggested by Monday I can probably get this in to the 1.10.4 release.

This adds an abstract BranchOperator, which implementors can use to create concrete operators for branching workflows fully encapsulated in a class, rather than needing to pass an external callable (or self-reference) to BranchPythonOperator. Also adds tests for same.

This moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BranchOperator to use this mixin instead of implementing the logic themselves.

Adds a brief section on BranchOperator to docs/concepts.rst

chronitis · 2019-06-24T09:15:56Z

@ashb I think that should cover all the changes you have requested. Thanks for getting back to this one.

BranchOperator is prefixed Base to reflect that it is designed to be subclassed, and the documentation and tests updated accordingly.

ashb · 2019-06-24T13:19:19Z

(I edited your branch to fix the pylint warning)

kaxil · 2019-06-24T14:41:50Z

Good work @chronitis

This adds an abstract BaseBranchOperator, which implementors can use to create concrete operators for branching workflows fully encapsulated in a class, rather than needing to pass an external callable (or self-reference) to BranchPythonOperator. Also adds tests for same. It also moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BaseBranchOperator to use this mixin instead of implementing the logic themselves. (cherry picked from commit 3891be4)

ashb · 2019-06-24T21:05:58Z

@chronitis Can you see if I've made some silly mistake on cherry-picking this back to the release branch -- it's failing on it's Python 2 tests. 3fc191c

Come the morning I'll pull this out of the branch to get a 1.10.4beta1 out but hopefully we can get it back in before we create the first release candidate.

ashb · 2019-06-24T21:11:20Z

Oh got it - str (py3) vs basesxtring (py2)

This adds an abstract BaseBranchOperator, which implementors can use to create concrete operators for branching workflows fully encapsulated in a class, rather than needing to pass an external callable (or self-reference) to BranchPythonOperator. Also adds tests for same. It also moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BaseBranchOperator to use this mixin instead of implementing the logic themselves. (cherry picked from commit 3891be4)

This adds an abstract BaseBranchOperator, which implementors can use to create concrete operators for branching workflows fully encapsulated in a class, rather than needing to pass an external callable (or self-reference) to BranchPythonOperator. Also adds tests for same. It also moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BaseBranchOperator to use this mixin instead of implementing the logic themselves.

This adds an abstract BaseBranchOperator, which implementors can use to create concrete operators for branching workflows fully encapsulated in a class, rather than needing to pass an external callable (or self-reference) to BranchPythonOperator. Also adds tests for same. It also moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BaseBranchOperator to use this mixin instead of implementing the logic themselves. (cherry picked from commit 3891be4)

potiuk self-requested a review May 8, 2019 19:20

ashb requested changes Jun 21, 2019

View reviewed changes

ashb changed the title ~~[AIRFLOW-4456] Add BranchOperator~~ [AIRFLOW-4456] Add sub-classable BaseBranchOperator Jun 21, 2019

ashb requested changes Jun 21, 2019

View reviewed changes

docs/concepts.rst Outdated Show resolved Hide resolved

docs/concepts.rst Outdated Show resolved Hide resolved

docs/concepts.rst Outdated Show resolved Hide resolved

Gordon Ball added 3 commits June 24, 2019 11:08

[AIRFLOW-4456] Refactor branching logic into SkipMixin

e4c8bfe

This moves the logic of skipping non-selected branches into airflow.models.SkipMixin:skip_all_except, and modifies BranchPythonOperator and BranchOperator to use this mixin instead of implementing the logic themselves.

[AIRFLOW-4456] Document BranchOperator

9c4a726

Adds a brief section on BranchOperator to docs/concepts.rst

[AIRFLOW-4456] BranchOperator->BaseBranchOperator

64a1926

BranchOperator is prefixed Base to reflect that it is designed to be subclassed, and the documentation and tests updated accordingly.

ashb approved these changes Jun 24, 2019

View reviewed changes

Update branch_operator.py

070cd2a

kaxil approved these changes Jun 24, 2019

View reviewed changes

ashb merged commit 3891be4 into apache:master Jun 24, 2019

yuqian90 mentioned this pull request Feb 28, 2020

[AIRFLOW-6657] Deprecate BaseBranchOperator #7331

Closed

6 tasks

RosterIn mentioned this pull request Oct 29, 2020

BaseBranchOperator isn't used anywhere #11931

Closed

	class BranchOperator(BaseOperator, SkipMixin):
	class BaseBranchOperator(BaseOperator, SkipMixin):

[AIRFLOW-4456] Add sub-classable BaseBranchOperator #5231

[AIRFLOW-4456] Add sub-classable BaseBranchOperator #5231

Uh oh!

Conversation

chronitis commented May 3, 2019

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

ashb commented May 3, 2019

Uh oh!

ashb commented May 3, 2019

Uh oh!

chronitis commented May 3, 2019

Uh oh!

ashb commented May 3, 2019

Uh oh!

ashb commented May 3, 2019

Uh oh!

chronitis commented May 3, 2019

Uh oh!

chronitis commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chronitis commented May 8, 2019

Uh oh!

potiuk commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chronitis commented May 10, 2019

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

ashb Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashb commented Jun 21, 2019

Uh oh!

chronitis commented Jun 24, 2019

Uh oh!

ashb commented Jun 24, 2019

Uh oh!

kaxil commented Jun 24, 2019

Uh oh!

ashb commented Jun 24, 2019

Uh oh!

ashb commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chronitis commented May 3, 2019 •

edited

Loading

codecov-io commented May 7, 2019 •

edited

Loading

potiuk commented May 8, 2019 •

edited

Loading