[AIRFLOW-3207] option to stop task pushing result to xcom

[marengaz]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-3207] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-3207
  • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-3207], code changes always need a Jira issue.

Description

• Here are some details about my PR, including screenshots of any UI changes:
  BaseOperator should have an option to stop a task pushing the return value of execute() to xcom.
  the default should be to push (preserves backward compat)

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:
  test_save_result_to_xcom() tests the flag in false. previous tests already test the auto/true option

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.

Code Quality

• Passes flake8

[codecov-io]

Codecov Report

Merging #4056 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4056      +/-   ##
==========================================
+ Coverage   78.12%   78.13%   +<.01%     
==========================================
  Files         202      202              
  Lines       16483    16484       +1     
==========================================
+ Hits        12878    12879       +1     
  Misses       3605     3605

Impacted Files	Coverage Δ
airflow/models/__init__.py	92.68% <100%> (ø)	⬆️

---

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 7a6acbf...b3a2f5b. Read the comment docs.

[ashb]

See also Fokko's comment here #3981 (review) (you may have already)

I'd rather we used the same flag we already have elsewhere xcom_push, as that is what many of the other operators already use. Additionally we can remove this flag from any operators that set it and let them fallback to using this flag on BaseOperator.

You may need to co-ordinate with @itscaro on this change

[marengaz]

cheers @ashb - i wasn't aware of this pr, or that there were operators that already had this function built in individually!
i will change save_result_to_xcom to xcom_push

[marengaz]

@ashb - we cant use a flag BaseOperator.xcom_push because this conflicts with the method BaseOperator.xcom_push(). I'll change the flag to be BaseOperator.do_xcom_push. it's already like this in some operators

[marengaz]

flake8 check is failing due to lines i haven't edited. please can we merge?

probs also worth considering adding W504 to the flake8 ignore list / doing a bulk correct?

[itscaro]

True by default?

[ashb]

Since this is a global flag, it should be true by default to not change the behaviour of all existing operators that push.

I think?

[itscaro]

XCOM feature if not used should be disabled by default as in base operators, users should declare explicitly the use of XCOM so that database is not filled with useless data.

For backwards compatibility, contrib operators, which currently push to XCOM by default, should be updated to activate XCOM explicitly, doing this way, other contrib operators will keep the current behavior of not using XCOM.

[marengaz]

if i was rewriting airflow from scratch, i would agree with you @itscaro.
but in this instance, where airflow is looking to mature a bit, i agree with @ashb; we should preserve bw compat where possible.

[Fokko]

I agree with @ashb. For backward compatibility, it should be true by default.

[itscaro]

You can still make it backward compatible by adding True to the operators which have this behavior:

As what I tried to do in #3981

Or a branch over this PR: master...itscaro:do_xcom_push

There is no good to introduce this default behavior as True in the models

[ashb]

Airflow currently operates as If this value has True by default -- if I write a custom operator and return "foo" from it that will make it to XCom. This is the behaviour we need to keep.

[itscaro]

I missed the case about operators outside of this repository. We might start to make it defaults to False for operators in this repository, for other modules the default is True for backwards compatibility until a major version?

To have to disable XCOM in each operator call might be something not wanted.

[Fokko]

@itscaro You could set it in the default_args then.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ron819]

@itscaro What is the status of this PR?

[marengaz]

@ron819 this and #4345 are ready to go, im just waiting for a committer to merge it!

[Fokko]

@marengaz Thanks, merging to master!