Skip to content

[AIRFLOW-1814] : templatize PythonOperator op_args and op_kwargs fields #4691

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.

Sign up for GitHub

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

Jump to bottom
Merged
Fokko merged 2 commits into from
Feb 27, 2019
Merged

[AIRFLOW-1814] : templatize PythonOperator op_args and op_kwargs fields #4691

Fokko merged 2 commits into from
Feb 27, 2019

Conversation

@galak75
Copy link
Contributor

@galak75 galak75 commented Feb 11, 2019 •
edited
Loading

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    See JIRA AIRFLOW-1814 description

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    • test_python_operator.test_python_callable_arguments_are_templatized
    • test_python_operator.test_python_callable_keyword_arguments_are_templatized

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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
      Docstring updated

Code Quality

  • Passes flake8

@galak75 galak75 force-pushed the feature/AIRFLOW-1814-templatize-python-operator-parameters branch from a491110 to 7b14e08 Compare February 13, 2019 17:39
@galak75
Copy link
Contributor Author

galak75 commented Feb 13, 2019

I just rebased my branch onto master. Ready to have some feedback 😉

@galak75 galak75 force-pushed the feature/AIRFLOW-1814-templatize-python-operator-parameters branch from 7b14e08 to 4e071a5 Compare February 15, 2019 16:03
@codecov-io
Copy link

codecov-io commented Feb 15, 2019 •
edited
Loading

Codecov Report

Merging #4691 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4691      +/-   ##
==========================================
- Coverage   74.94%   74.94%   -0.01%     
==========================================
  Files         434      434              
  Lines       28392    28392              
==========================================
- Hits        21279    21278       -1     
- Misses       7113     7114       +1
Impacted Files Coverage Δ
airflow/operators/python_operator.py 95.26% <100%> (ø) ⬆️
airflow/contrib/operators/ssh_operator.py 82.05% <0%> (-1.29%) ⬇️

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 6c64b1d...9da5360. Read the comment docs.

@mik-laj
Copy link
Member

mik-laj commented Feb 15, 2019

@galak75 I created a PR with suggestion: VilledeMontreal#4

@galak75
Copy link
Contributor Author

galak75 commented Feb 15, 2019 •
edited
Loading

@galak75 I created a PR with suggestion: VilledeMontreal#4

@mik-laj: I was initially using unittest.Mock, but my tests were failing with the following error: TypeError: Object of type Mock is not JSON serializable
I will try to apply your suggestion. For sure I would rather use mock object rather than my custom Call class

I'll let you know as soon as tested
Thank you

@mik-laj
Copy link
Member

mik-laj commented Feb 15, 2019

Oh. You're right. It may not work. I did not check it thoroughly. This construction seemed surprising to me at first. Maybe you add a comment in the code so that others can better understand the purpose of this class?

@galak75
Copy link
Contributor Author

galak75 commented Feb 16, 2019

I checked, and it actually fails with a TypeError: Object of type Mock is not JSON serializable
see https://travis-ci.org/VilledeMontreal/incubator-airflow/builds/494081085

You're right, this test code seems awkward at first. Using a Mock would be a better solution.
Maybe someone knows how to make it work with a Mock?
Meanwhile I'm adding some comment to explain this unusual test code choice.

@galak75 galak75 force-pushed the feature/AIRFLOW-1814-templatize-python-operator-parameters branch 4 times, most recently from fff1b51 to 213ee3d Compare February 20, 2019 15:17
@galak75
Copy link
Contributor Author

galak75 commented Feb 20, 2019

@mik-laj : do you have any additional suggestion on comments I've added to the code? Any other improvement to the code?

@galak75 galak75 force-pushed the feature/AIRFLOW-1814-templatize-python-operator-parameters branch 2 times, most recently from 3dabd5e to 0852b68 Compare February 22, 2019 15:57
@galak75
[AIRFLOW-1814] : templatize PythonOperator op_args and op_kwargs fields
f15b3e9 
[AIRFLOW-1814] : add assertCallsEqual test utility method

[AIRFLOW-1814] : rename test utility class

[AIRFLOW-1814] : remove parameter set with default value from test call

[AIRFLOW-1814] : make test utility method private

[AIRFLOW-1814] : change test values

[AIRFLOW-1814] : change test code format

[AIRFLOW-1814] : remove parameter types (python2 backward compatibility)

[AIRFLOW-1814] : add (templated) marker to changed attributes
@galak75
[AIRFLOW-1814] comment some awkward test code
9da5360
@galak75 galak75 force-pushed the feature/AIRFLOW-1814-templatize-python-operator-parameters branch from 0852b68 to 9da5360 Compare February 22, 2019 16:59
Fokko
Fokko approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Looks good @galak75, I think it is good to be able to template the op_args and op_kwargs. Cheers

@Fokko Fokko merged commit 7ab245b into apache:master Feb 27, 2019
@galak75
Copy link
Contributor Author

galak75 commented Feb 27, 2019

Thanks a lot @Fokko!

ashb pushed a commit to ashb/airflow that referenced this pull request Mar 27, 2019
@galak75 @ashb
[AIRFLOW-1814] : Temple PythonOperator {op_args,op_kwargs} fields (ap…
c74f6c3 
…ache#4691)
@galak75 galak75 deleted the feature/AIRFLOW-1814-templatize-python-operator-parameters branch March 29, 2019 17:29
@OmerJog
Copy link
Contributor

OmerJog commented Jun 27, 2019

There is a report that this PR is breaking change:
https://issues.apache.org/jira/browse/AIRFLOW-4451
@galak75 can you address this?

@galak75
Copy link
Contributor Author

galak75 commented Jul 3, 2019

There is a report that this PR is breaking change:
https://issues.apache.org/jira/browse/AIRFLOW-4451
@galak75 can you address this?

I will try to

andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
@galak75 @andriisoldatenko
[AIRFLOW-1814] : Temple PythonOperator {op_args,op_kwargs} fields (ap…
b85f0a5 
…ache#4691)
@AetherUnbound AetherUnbound mentioned this pull request Jul 28, 2019
Merged
6 tasks
wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019
@galak75
[AIRFLOW-1814] : Temple PythonOperator {op_args,op_kwargs} fields (ap…
e75da63 
…ache#4691)
@mik-laj mik-laj mentioned this pull request May 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fokko Fokko Fokko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@galak75 @codecov-io @mik-laj @OmerJog @Fokko