amoghrajesh commented on code in PR #63871:
URL: https://github.com/apache/airflow/pull/63871#discussion_r3182120340
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -32,7 +32,7 @@
from airflow.timetables.base import Timetable as CoreTimetable
-def serialize_template_field(template_field: Any, name: str) -> str | dict |
list | int | float:
+def serialize_template_field(template_field: Any, name: str) -> str | dict |
list | int | float | bool:
Review Comment:
There is a version of this function present in `task_runner.py` too called:
`_serialize_template_field`, do we need this change there as well to keep them
in sync?
##########
airflow-core/tests/unit/serialization/test_dag_serialization.py:
##########
@@ -704,6 +706,16 @@ def test_deserialization_across_process(self):
for dag_id in stringified_dags:
self.validate_deserialized_dag(stringified_dags[dag_id],
dags[dag_id])
+ @conf_vars({("core", "load_examples"): "false"})
Review Comment:
`@pytest.mark.db_test` is needed too?
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -51,55 +51,42 @@ def is_jsonable(x):
else:
return True
- def translate_tuples_to_lists(obj: Any):
- """Recursively convert tuples to lists."""
- if isinstance(obj, tuple):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, list):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, dict):
- return {key: translate_tuples_to_lists(value) for key, value in
obj.items()}
- return obj
+ def serialize_object(obj):
+ if obj is None or isinstance(obj, (str, int, float, bool)):
+ return obj
- def sort_dict_recursively(obj: Any) -> Any:
- """Recursively sort dictionaries to ensure consistent ordering."""
if isinstance(obj, dict):
- return {k: sort_dict_recursively(v) for k, v in
sorted(obj.items())}
- if isinstance(obj, list):
- return [sort_dict_recursively(item) for item in obj]
- if isinstance(obj, tuple):
- return tuple(sort_dict_recursively(item) for item in obj)
- return obj
+ # Sort dictionaries recursively to ensure consistent string
representation
+ # This prevents hash inconsistencies when dict ordering varies
+ return {
+ k: serialize_object(v)
+ for k, v in sorted(obj.items(), key=lambda kv:
(type(kv[0]).__name__, repr(kv[0])))
+ }
+
+ if isinstance(obj, (list, tuple)):
+ return [serialize_object(item) for item in obj]
+
+ if (serialize_attr := getattr(type(obj), "serialize", None)) and
callable(serialize_attr):
+ return serialize_object(obj.serialize())
+
+ if callable(obj):
+ # to make callable object (e.g. lambda function) static
Review Comment:
This comment just restates what the line does, maybe add the reasoning why
stable serialising is needed instead?
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -51,55 +51,42 @@ def is_jsonable(x):
else:
return True
- def translate_tuples_to_lists(obj: Any):
- """Recursively convert tuples to lists."""
- if isinstance(obj, tuple):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, list):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, dict):
- return {key: translate_tuples_to_lists(value) for key, value in
obj.items()}
- return obj
+ def serialize_object(obj):
+ if obj is None or isinstance(obj, (str, int, float, bool)):
+ return obj
- def sort_dict_recursively(obj: Any) -> Any:
- """Recursively sort dictionaries to ensure consistent ordering."""
if isinstance(obj, dict):
- return {k: sort_dict_recursively(v) for k, v in
sorted(obj.items())}
- if isinstance(obj, list):
- return [sort_dict_recursively(item) for item in obj]
- if isinstance(obj, tuple):
- return tuple(sort_dict_recursively(item) for item in obj)
- return obj
+ # Sort dictionaries recursively to ensure consistent string
representation
+ # This prevents hash inconsistencies when dict ordering varies
+ return {
+ k: serialize_object(v)
+ for k, v in sorted(obj.items(), key=lambda kv:
(type(kv[0]).__name__, repr(kv[0])))
+ }
+
+ if isinstance(obj, (list, tuple)):
+ return [serialize_object(item) for item in obj]
+
+ if (serialize_attr := getattr(type(obj), "serialize", None)) and
callable(serialize_attr):
Review Comment:
```suggestion
if (_ := getattr(type(obj), "serialize", None)) and
callable(serialize_attr):
```
Dead code otherwise.
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -51,55 +51,42 @@ def is_jsonable(x):
else:
return True
- def translate_tuples_to_lists(obj: Any):
- """Recursively convert tuples to lists."""
- if isinstance(obj, tuple):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, list):
- return [translate_tuples_to_lists(item) for item in obj]
- if isinstance(obj, dict):
- return {key: translate_tuples_to_lists(value) for key, value in
obj.items()}
- return obj
+ def serialize_object(obj):
+ if obj is None or isinstance(obj, (str, int, float, bool)):
+ return obj
- def sort_dict_recursively(obj: Any) -> Any:
- """Recursively sort dictionaries to ensure consistent ordering."""
if isinstance(obj, dict):
- return {k: sort_dict_recursively(v) for k, v in
sorted(obj.items())}
- if isinstance(obj, list):
- return [sort_dict_recursively(item) for item in obj]
- if isinstance(obj, tuple):
- return tuple(sort_dict_recursively(item) for item in obj)
- return obj
+ # Sort dictionaries recursively to ensure consistent string
representation
+ # This prevents hash inconsistencies when dict ordering varies
+ return {
+ k: serialize_object(v)
+ for k, v in sorted(obj.items(), key=lambda kv:
(type(kv[0]).__name__, repr(kv[0])))
+ }
+
+ if isinstance(obj, (list, tuple)):
+ return [serialize_object(item) for item in obj]
+
+ if (serialize_attr := getattr(type(obj), "serialize", None)) and
callable(serialize_attr):
Review Comment:
You can also just do:
```
callable(getattr(type(obj), "serialize", None)).
```
##########
airflow-core/tests/unit/dags/test_dag_decorator_version.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
Review Comment:
Intentionally added?
--
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]
