You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "utkarsharma2 (via GitHub)" <gi...@apache.org> on 2023/06/13 05:45:31 UTC
[GitHub] [airflow] utkarsharma2 opened a new pull request, #31866: Decode old Xcom value from airflow 2.5.2
utkarsharma2 opened a new pull request, #31866:
URL: https://github.com/apache/airflow/pull/31866
Xcom values generated by airflow 2.5.2 was not getting decoded by airflow 2.6.
closes: https://github.com/apache/airflow/issues/31769
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1229519074
##########
tests/utils/test_json.py:
##########
@@ -111,6 +111,23 @@ def test_encode_xcom_with_nested_dict(self, data):
e = json.loads(i, cls=utils_json.XComDecoder)
assert data == e
+ @pytest.mark.parametrize(
+ "data",
+ [
+ '[{"__classname__": "airflow.datasets.Dataset", "__version__": 1,'
+ ' "__data__": {"__var": {"uri": "bq://google_cloud_default@?table=table_name&schema=schema_name",'
+ ' "extra": null}, "__type": "dict"}}]'
+ ],
+ )
+ def test_decode_xcom_with_old_data(self, data):
+ """
+ Ensure that the Xcom value encoded by Airflow 2.5.2 is getting decoded by Airflow 2.6.
+ """
+ d = json.loads(data, cls=utils_json.XComDecoder)
+ assert d == [
+ Dataset(uri="bq://google_cloud_default@?table=table_name&schema=schema_name", extra=None)
+ ]
+
Review Comment:
If there is a test for the old code, this test should be moved to that test, and if not, we need to add a new test case in this one to cover the "else" block
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1228153978
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
+ return old[OLD_DATA]
+ else:
+ return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
Review Comment:
Sure, I'll try that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1230454223
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
+ return old[OLD_DATA]
+ else:
+ return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
Review Comment:
@ephraimbuddy Varified it with below xcom data produced by `Airflow 2.5.2` Dag id - `dataset_produces_1`
Xcom Value:
`[{"__classname__": "airflow.datasets.Dataset", "__version__": 1, "__data__": {"__var": {"uri": "s3://dag1/output_1.txt", "extra": {"__var": {"hi": "bye"}, "__type": "dict"}}, "__type": "dict"}}]`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1236963559
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
I'm not sure what all airflow version generates this encoded value, but I'll check and update that here. Sorry, should have done this in 1st iteration.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1245077973
##########
tests/serialization/test_serde.py:
##########
@@ -264,14 +264,33 @@ def test_backwards_compat(self):
"__var": {
"__var": {
"uri": uri,
- "extra": None,
+ "extra": {"__var": {"hi": "bye"}, "__type": "dict"},
},
"__type": "dict",
},
}
dataset = deserialize(data)
assert dataset.uri == uri
+ def test_backwards_compat_airflow_252(self):
+ """
+ Ensure that the Xcom value encoded by Airflow 2.5.2 is getting decoded by Airflow 2.6.
Review Comment:
@hussein-awala Updated the comment, can you please check now? :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1231371492
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
same thought ([comment](https://github.com/apache/airflow/pull/31866#pullrequestreview-1482133449))
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bolkedebruin merged pull request #31866: Decode old-style nested Xcom value
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin merged PR #31866:
URL: https://github.com/apache/airflow/pull/31866
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on PR #31866:
URL: https://github.com/apache/airflow/pull/31866#issuecomment-1601011965
@hussein-awala In Airflow 2.4.3 if you run dag `dataset_produces_1` it will produce xcom data as below, with nested `__var` -
```
{
"__type": "airflow.datasets.Dataset",
"__source": None,
"__var": {
"__var": {
"uri": uri,
"extra": {
"__var": {"hi": "bye"},
"__type": "dict"
}
},
"__type": "dict",
},
}
```
And I think `old[OLD_DATA][OLD_DATA]` was done to directly get to the second nested `__var` assuming there will be no more nested `__var` in serialized data but that is not the case as extra can have dicts too.
Also, in the old test case, we had only tests for the below data -
test case - https://github.com/apache/airflow/blob/main/tests/serialization/test_serde.py#L259-L273
```
{
"__type": "airflow.datasets.Dataset",
"__source": None,
"__var": {
"__var": {
"uri": uri,
"extra": None # <-- change
},
"__type": "dict",
},
}
```
`old[OLD_DATA][OLD_DATA]` fails with extra = {"__var": {"hi": "bye"}, "__type": "dict"} and will works with extra=None
But if we explicitly handle the case for dict and do not assume the nested levels of `__var` below code will work in all cases.
```
def _convert(old: dict) -> dict:
"""Converts an old style serialization to the new style.""
if OLD_TYPE in old and OLD_DATA in old:
#Added to handle for handling xcom data from airflow < 2.6
if old[OLD_TYPE] == "dict":
return old[OLD_DATA]
else:
return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
return old
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1237169658
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
I think the code comment should be applicable for 2.4>= to <2.6 because the dataset concept was introduced in 2.4. Will update accordingly. Will update the code comment accordingly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bolkedebruin commented on pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #31866:
URL: https://github.com/apache/airflow/pull/31866#issuecomment-1612717192
Looking
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1230454223
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
+ return old[OLD_DATA]
+ else:
+ return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
Review Comment:
@ephraimbuddy Verified it with below xcom data produced by `Airflow 2.5.2` Dag id - `dataset_produces_1`
Xcom Value:
`[{"__classname__": "airflow.datasets.Dataset", "__version__": 1, "__data__": {"__var": {"uri": "s3://dag1/output_1.txt", "extra": {"__var": {"hi": "bye"}, "__type": "dict"}}, "__type": "dict"}}]`
Produced dataset:
`[Dataset(uri='s3://dag1/output_1.txt', extra={'hi': 'bye'})]`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1231370161
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
```suggestion
# Added to handle for handling xcom data from airflow 2.5.2
```
This comment feels a little weird to me. Is this really just 2.5.2, or <2.6?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1237173782
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
Also, I was able to replicate the issue for airflow 2.4.3 as running example dag `dataset_produces_1` produces data - `[{“__type": "airflow.datasets.Dataset", "__source": "dataset_produces_1.producing_task_1", "__var": {"__var": {"uri": "s3://dag1/output_1.txt", "extra": {"__var": {"hi": "bye"}, "__type": "dict"}}, "__type": "dict"}}]` and the issue was not caught by testcase - https://github.com/apache/airflow/blob/main/tests/serialization/test_serde.py#L259-L273 as it doesn't have more than two levels of nested `__var`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bolkedebruin commented on pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #31866:
URL: https://github.com/apache/airflow/pull/31866#issuecomment-1612788272
`_convert` was created to handle old (pre 2.6) XCom seralized data, but the issue is that the new deserializer cannot handle:
```
{'extra': {'__type': 'dict', '__var': {'hi': 'bye'}}, 'uri': 's3://does_not_exist'}
```
Which is the nested form of the old style serialization.
I think the solution is correct, but that the commit message and the inline comments could be improved. As mentioned this code was already handling pre 2.6 data, but it wasn't handling the nested version of that correctly. Also I think the test could use some improvement / addition. The above `dict` should be able to be deseralized directly. So I would expect an additional test.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1230477559
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
Review Comment:
Added code comments.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1230454223
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
+ return old[OLD_DATA]
+ else:
+ return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
Review Comment:
@ephraimbuddy Varified it with below xcom data produced by `Airflow 2.5.2` Dag id - `dataset_produces_1`
Xcom Value:
`[{"__classname__": "airflow.datasets.Dataset", "__version__": 1, "__data__": {"__var": {"uri": "s3://dag1/output_1.txt", "extra": {"__var": {"hi": "bye"}, "__type": "dict"}}, "__type": "dict"}}]`
Produced dataset:
`[Dataset(uri='s3://dag1/output_1.txt', extra={'hi': 'bye'})]`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] sunank200 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1227666783
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
Review Comment:
I think adding a comment here why we are comparing with `dict` would help
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1236963559
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ # Added to handle for handling xcom data from airflow 2.5.2
Review Comment:
I'm not sure which airflow version generates this encoded value, but I'll check and update that here. Sorry, should have done this in 1st iteration.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1241253574
##########
tests/serialization/test_serde.py:
##########
@@ -264,14 +264,33 @@ def test_backwards_compat(self):
"__var": {
"__var": {
"uri": uri,
- "extra": None,
+ "extra": {"__var": {"hi": "bye"}, "__type": "dict"},
},
"__type": "dict",
},
}
dataset = deserialize(data)
assert dataset.uri == uri
+ def test_backwards_compat_airflow_252(self):
+ """
+ Ensure that the Xcom value encoded by Airflow 2.5.2 is getting decoded by Airflow 2.6.
Review Comment:
I think you need to update this comment, since we have the same format since 2.4.3, you can just say < 2.6, wdyt?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1227684455
##########
airflow/serialization/serde.py:
##########
@@ -276,7 +276,10 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
def _convert(old: dict) -> dict:
"""Converts an old style serialization to new style."""
if OLD_TYPE in old and OLD_DATA in old:
- return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+ if old[OLD_TYPE] == "dict":
+ return old[OLD_DATA]
+ else:
+ return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
Review Comment:
Can we verify this by producing xcom in 2.5.2 and upgrade to 2.6.1 with this change?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31866: Decode old Xcom value from airflow 2.5.2
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31866:
URL: https://github.com/apache/airflow/pull/31866#discussion_r1230479865
##########
tests/utils/test_json.py:
##########
@@ -111,6 +111,23 @@ def test_encode_xcom_with_nested_dict(self, data):
e = json.loads(i, cls=utils_json.XComDecoder)
assert data == e
+ @pytest.mark.parametrize(
+ "data",
+ [
+ '[{"__classname__": "airflow.datasets.Dataset", "__version__": 1,'
+ ' "__data__": {"__var": {"uri": "bq://google_cloud_default@?table=table_name&schema=schema_name",'
+ ' "extra": null}, "__type": "dict"}}]'
+ ],
+ )
+ def test_decode_xcom_with_old_data(self, data):
+ """
+ Ensure that the Xcom value encoded by Airflow 2.5.2 is getting decoded by Airflow 2.6.
+ """
+ d = json.loads(data, cls=utils_json.XComDecoder)
+ assert d == [
+ Dataset(uri="bq://google_cloud_default@?table=table_name&schema=schema_name", extra=None)
+ ]
+
Review Comment:
@hussein-awala Moved test case to `airflow/tests/serialization/test_serde.py` and the else part is covered by this testcase - https://github.com/apache/airflow/blob/a81ac70b33a589c58b59864df931d3293fada382/tests/serialization/test_serde.py#L259
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org