-
Notifications
You must be signed in to change notification settings - Fork 15.1k
Treat single task_ids in xcom_pull the same as multiple #49692
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.
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
Conversation
TODO: doc changes and test addition (theres no test that specifically tests this portion) |
Cool, yeah we should remove the entry we added for 3.0.0 https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html#behaviour-change-in-xcom-pull -- when this PR is ready like you mentioned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this code duplication, the following diff should work:
diff --git a/task-sdk/src/airflow/sdk/execution_time/task_runner.py b/task-sdk/src/airflow/sdk/execution_time/task_runner.py
index a34a4c78ff..706a72b378 100644
--- a/task-sdk/src/airflow/sdk/execution_time/task_runner.py
+++ b/task-sdk/src/airflow/sdk/execution_time/task_runner.py
@@ -325,6 +325,9 @@ class RuntimeTaskInstance(TaskInstance):
if run_id is None:
run_id = self.run_id
+ single_task_requested = isinstance(task_ids, (str, type(None)))
+ single_map_index_requested = isinstance(map_indexes, (int, type(None), ArgNotSet))
+
if task_ids is None:
# default to the current task if not provided
task_ids = [self.task_id]
@@ -363,7 +366,7 @@ class RuntimeTaskInstance(TaskInstance):
else:
xcoms.append(value)
- if len(xcoms) == 1:
+ if single_task_requested and single_map_index_requested:
return xcoms[0]
return xcoms
Umm yeah we can do that, i didnt want to overly simplify it that much, but seemingly it looks good! |
@kaxil I updated it, it passes all my tests too, so we are good. |
Cool, yeah the code duplication in my opinion made it look complex and having too follow two entirely separate branches too. This keep the flow same. |
Nice! |
efc7385
to
55038ed
Compare
Fixed doc change in 55038ed |
Thank you! @kaxil I didnt get time to do this earlier |
…49692) * Treat single task_ids in xcom_pull the same as multiple closes: #49540 * fixup! Treat single task_ids in xcom_pull the same as multiple --------- (cherry picked from commit 517b29d) Co-authored-by: Amogh Desai <[email protected]> Co-authored-by: Kaxil Naik <[email protected]>
…pache#49692) * Treat single task_ids in xcom_pull the same as multiple closes: apache#49540 * fixup! Treat single task_ids in xcom_pull the same as multiple --------- (cherry picked from commit 517b29d) Co-authored-by: Amogh Desai <[email protected]> Co-authored-by: Kaxil Naik <[email protected]>
…49692) (#49820) * Treat single task_ids in xcom_pull the same as multiple closes: #49540 * fixup! Treat single task_ids in xcom_pull the same as multiple --------- (cherry picked from commit 517b29d) Co-authored-by: Amogh Desai <[email protected]> Co-authored-by: Kaxil Naik <[email protected]>
* Treat single task_ids in xcom_pull the same as multiple closes: apache#49540 * fixup! Treat single task_ids in xcom_pull the same as multiple --------- Co-authored-by: Kaxil Naik <[email protected]>
closes: #49540
Problem
There is no need to handle some cases of xcom retrieval seperately.
Expected behaviour in airflow 2:
LazyXComSelectSequence
object which was to avoid pulling everything into memory at once (unlike session.execute(...).fetchall()). But this is no longer relevant as we get the xcoms we need to from the api server (usingget_one
from BaseXcom class).So we need to have a similar abstraction so that the user code doesn't have to change when they called xcom_pull using single task_id in a list, like
ti.xcom_pull(task_ids=["task1"])
.xcom_pull(task_ids="task1") should return a value and not a list of values
xcom_pull with one task_id and one map_indexes should return a value again and not an iterable
Testing
Testing with various combinations of task_ids
DAG used for testing:
This DAG tests three things:
Testing with map_indexes
DAG:
Observe that each value is pushed as a single value and we get it normally now.
Similar behaviour to AF2:
We return a list instead of a
LazySelectSequence
though^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in airflow-core/newsfragments.