[AIRFLOW-6392] Remove cyclic dependency baseoperator <-> helpers

[potiuk]

There is a hidden cyclic dependency between baseoperator and helpers module.
It's hidden by local import but it is detected when baseoperator/helpers are
removed from pylint_todo.txt (and it's really there).

The dependency comes from BaseOperator using helpers and two helpers methods
(chain and cross_downstream) using BaseOperator. This can be solved by
converting the chain and cross_downstream methods to be static methods in
BaseOperator class.

• Description above provides context of the change
• Commit message contains [AIRFLOW-6392] or [AIRFLOW-XXXX] for document-only changes
• Unit tests coverage for changes (not needed for documentation changes)
• Commits follow "How to write a good git commit message"
• Relevant documentation is updated including usage instructions.
• I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

[codecov-io]

Codecov Report

Merging #6950 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6950      +/-   ##
==========================================
- Coverage   84.81%   84.53%   -0.29%     
==========================================
  Files         679      679              
  Lines       38495    38618     +123     
==========================================
- Hits        32649    32645       -4     
- Misses       5846     5973     +127

Impacted Files	Coverage Δ
airflow/utils/helpers.py	81.69% <ø> (-1.06%)	⬇️
...low/example_dags/example_short_circuit_operator.py	100% <100%> (ø)	⬆️
airflow/models/baseoperator.py	96.25% <100%> (+0.17%)	⬆️
airflow/kubernetes/volume_mount.py	44.44% <0%> (-55.56%)	⬇️
airflow/kubernetes/volume.py	52.94% <0%> (-47.06%)	⬇️
airflow/kubernetes/pod_launcher.py	45.25% <0%> (-46.72%)	⬇️
airflow/kubernetes/refresh_config.py	50.98% <0%> (-23.53%)	⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py	78.75% <0%> (-20%)	⬇️
airflow/contrib/operators/spark_submit_operator.py	90% <0%> (-2.86%)	⬇️
airflow/contrib/hooks/spark_submit_hook.py	83.06% <0%> (+0.98%)	⬆️

---

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 4bfde02...2d5c38f. Read the comment docs.

[potiuk]

I'd love some other review as well for that one, I am moving some of the "helper" methods to BaseOperator's static methods so it would be great to see if others have an opinion on that one :).

[potiuk]

And we have a follow-up substantial pylint update on top of that, so it would be great to merge it rather quickly.

[mik-laj]

These functions are used by users, so we should think about backward compatibility.

[mik-laj]

CC: @zhongjiajie

[mik-laj]

Related PR:
#4779
e732006#comments

[mik-laj]

These functions are used by users, so we should think about backward compatibility.

[potiuk]

These functions are used by users, so we should think about backward compatibility.

Yep. Agree - that's why i called others :)

[potiuk]

@mik-laj -> moved the methods back to be module methods (in baseoperator module) also added more description in UPDATING.

I would love to hear what others think about the need of keeping the backwards compatibility here. Seems that we can't agree with @mik-laj whether it's important or not in this case.

[kaxil]

Do we need this? I don't think we need this check. We can disable this check on our Pycharm/Intelij

[potiuk]

I am afraid so. The check is super useful because it detects spelling mistakes in parameters or removed parameters.

This is actually a known problem because the memo parameter is by mistake missing in skeleton definition for python skeletons (from typeshed I believe). The issue is described
here: https://intellij-support.jetbrains.com/hc/en-us/community/posts/360000181224-deepcopy-unexpected-memo-argument
and reported here (unresolved): https://youtrack.jetbrains.com/issue/PY-29664

I have not found a good way of silencing it only for deepcopy, so I propose we can leave it as it is.

[potiuk]

Updated all comments @kaxil ! What shall we do with the backwards compatibility there? Do you think we should worry about this in this case?

[kaxil]

Updated all comments @kaxil ! What shall we do with the backwards compatibility there? Do you think we should worry about this in this case?

#6950 (comment) - I think this one is still pending.

I think this is the chance to make it right so I won't worry much about backward-compatibility here.

We won't merge this into 1.10.*

[potiuk]

@mik-laj ?

[turbaszek]

@potiuk can you rebase?

[zhongjiajie]

These functions are used by users, so we should think about backward compatibility.

I submit #4779 due to use bit shift will so lengthy in some situation as I comment in #4779 (comment), but when I use chain could be more easy using chain(*[lists_of_task]), So IMO and situation, I think function chain is useful and I will not remove it, that my point

[zhongjiajie]

Our community have another function chain big fans @bcb , FYI, chain import path will change in Ariflow 2.0

[zhongjiajie]

@potiuk Should we add remind user different import path in https://github.com/apache/airflow/blob/master/docs/concepts.rst#relationship-helper for different Airflow version? Besides, code looking good for me

[zhongjiajie]

Updated all comments @kaxil ! What shall we do with the backwards compatibility there? Do you think we should worry about this in this case?

IMO, If only import path change but function still exists, I think is not a problem. But if remove function should be more careful.

[potiuk]

@zhongjiajie -> I added a note in concepts (and changed the name to "Relationship builders" :).