Change BaseOperatorLink interface to take a ti_key, not a datetime #21798
Conversation
3bde371 to
4d2fd52
Compare
airflow/models/abstractoperator.py
Outdated
There was a problem hiding this comment.
@uranusjr Do you think this is enough of a check, or should we force ti_key to be accepted only as a keyword argument?
Is it worth doing a __init_subclass__ check on BaseOperatorLink to check the signature?
There was a problem hiding this comment.
I think this should be good enough. Can’t think of all the future possibilities but this should be strict enough, if we always call get_link with keyword ti_key (we already do here).
There was a problem hiding this comment.
This test was broadly a duplicate of test_extra_serialized_field_and_operator_links -- so I made that parameterized instead.
There was a problem hiding this comment.
New base class for as almost all of the links in Google provider follow this simple pattern.
uranusjr
left a comment
There was a problem hiding this comment.
All minor thoughts, generally looks good to me.
airflow/models/abstractoperator.py
Outdated
There was a problem hiding this comment.
I think this should be good enough. Can’t think of all the future possibilities but this should be strict enough, if we always call get_link with keyword ti_key (we already do here).
airflow/operators/trigger_dagrun.py
Outdated
There was a problem hiding this comment.
Continuing the backward compatibility topic, this particular class may not need to implement the compatibility signature? If we don’t support getting the link through the Python API, this one can only be called by Airflow core and only with the new API. (The same does not apply for operators in providers, of course.)
There was a problem hiding this comment.
You make a good point. I was just doing all of them without paying that much attention where the operator was defined.
There was a problem hiding this comment.
Oh,
airflow/operators/trigger_dagrun.py:51: error: Signature of "get_link"
incompatible with supertype "BaseOperatorLink" [override]
def get_link(
^
Maybe the override doesn't actually hurts us here?
There was a problem hiding this comment.
I'm trying to remove the overload, if that doesn't cause typing problems for the providers I'll leave it just with the new signature.
There was a problem hiding this comment.
Just implementing the new signature should work fine. We could also implement the compatibility signature and ignore the compatibility arguments (with a note explaining this is fine).
a43f108 to
1eed46e
Compare
|
Would it be a good idea if |
I agree, I've done it as part of 2edebe2 in #21710 (for no real reason, other than we are making larger changes there) -- this PR was concerned with just the changes needed for OperatorLink interface. |
We want to encourage people to use the new format, so only show the new one.
8dae837 to
6fc80d5
Compare
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
|
Failure is just a timeout cos of
|
In 2.2 we "deprecated" passing an execution date to XCom.get methods, but there was no other option for operator links.
Now in 2.3 as part of Dynamic Task Mapping (AIP-42) we will need to add map_index to the XCom row to support the "reduce" part of the API.
In order to support that cleanly we need to change the interface for BaseOperatorLink to take an identifier (as dttm/execution_date + task is no longer unique)
This PR adds support for passing a
ti_keyto operator link callbacks if the signature supports it.Todo: