shahar1 commented on PR #66712:
URL: https://github.com/apache/airflow/pull/66712#issuecomment-4424585768
> > @Dev-iL - I'll appreciate your feedback here what went it wrong, and how
we could prevent it in the next iteration.
>
> The answer seems quite obvious - either we had no tests for this, or the
right tests didnt run.
>
> Would it be possible to revert only the changes that actually break
something and not everything?
I checked, and there's indeed an issue in the rewrite: `task.output["key"]`
isn't always equivalent to `{{ xcom_pull('task')['key'] }}`.
See for example the following snippet:
```python
@task
def make_dict():
return {"key": "value"} # Pushes one XCom row: key="return_value",
value={"key": "value"}
t = make_dict()
```
In the old form, Jinja is rendered at runtime:
```
"{{ ti.xcom_pull('make_dict')['key'] }}"
# 1. ti.xcom_pull('make_dict') -> reads
key="return_value" → {"key": "value"}
# 2. ['key'] -> Python dict-index
→ "value"
```
However, in the new form, `XComArg` is resolved at runtime:
```
t.output["key"]
# 1. t.output ->
PlainXComArg(op=make_dict, key="return_value")
# 2. ["key"] ->
PlainXComArg(op=make_dict, key="key") <- key swapped, NOT indexed
# 3. resolve() ->
ti.xcom_pull(task_ids="make_dict", key="key")
# 4. no row with key="key" exists -> None
```
See following for reference:
https://github.com/apache/airflow/blob/15d297af1948ab7f310ce87dfc673a1379dd9b7c/task-sdk/src/airflow/sdk/definitions/xcom_arg.py#L220
The reasons that the system tests didn't fail is that we currently don't run
them in the CI, as they require integration with the Cloud providers
(specifically in GCP we don't have it). For that reason, GCP system tests
currently run on Vlada's team's instance. The monitoring for these tests is
publicly available:
https://storage.googleapis.com/providers-dashboard-html/dashboard.html
However, we currently don't have a mechanism to detect issues in PRs except
for running the system tests locally (which wasn't done during merging - I
should have checked it when reviewing last PR).
For the reasons above, I'll revert the changes as suggested (including in
all providers) - if you manage to solve the issue described above, including
unit tests and working example dags (could be simplified versions, no need for
GCP logic), I'll happily revert the revert (only someone has to test at least
one Dag).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
