kaxil commented on code in PR #65705:
URL: https://github.com/apache/airflow/pull/65705#discussion_r3131253239
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -359,6 +359,22 @@ def deserialize(cls, data: dict, dags: dict) -> DagParam:
return cls(current_dag=current_dag, name=data["name"],
default=data["default"])
+ def __repr__(self) -> str:
Review Comment:
This treats the symptom, not the root cause. `serialize_template_field`
calls `str()` on tuples/lists when `is_jsonable` returns False (helpers.py:98),
and the problem is that path doesn't recurse into elements that have their own
`.serialize()`. Any class that can land inside a template-field tuple without a
stable repr hits the same bug -- `DagParam` just happens to be the one in this
repro.
Other classes already handle this via `@attrs.define` (see `MappedArgument`
in `expandinput.py` and `XComArg` subclasses). Have you considered either (a)
converting `DagParam` to `@attrs.define` for consistency with the rest of the
SDK, or (b) fixing `serialize_template_field` to call `.serialize()` on
tuple/list elements that expose one, similar to #63871's approach? `DagParam`
already has a `serialize()` method at line 338 that would do the right thing.
##########
airflow-core/tests/unit/serialization/test_dag_serialization.py:
##########
@@ -702,6 +703,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"})
+ def test_reserialize_should_make_equal_hash_with_dag_processor(self):
+ dagbag1 = DagBag(TEST_DAGS_FOLDER / "test_dag_decorator_version.py")
+ result1 = DagSerialization.to_dict(next(iter(dagbag1.dags.values())))
+
+ dagbag2 = DagBag(TEST_DAGS_FOLDER / "test_dag_decorator_version.py")
+ result2 = DagSerialization.to_dict(next(iter(dagbag2.dags.values())))
+
Review Comment:
This test asserts the serialized dict is identical across two parses, which
proves the stringification is stable. But the actual user-visible bug is DAG
version inflation, which is driven by the DAG hash/fingerprint, not the raw
dict.
Could you add an assertion on the DAG hash (or whatever field the
version-change detector compares) to tie the test directly to the reported
symptom? That way a future regression in a different field producing unstable
output would still be caught here.
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -359,6 +359,22 @@ def deserialize(cls, data: dict, dags: dict) -> DagParam:
return cls(current_dag=current_dag, name=data["name"],
default=data["default"])
+ def __repr__(self) -> str:
+ return json.dumps(
+ {
+ "dag_id": self.current_dag.dag_id,
+ "name": self._name,
+ }
+ )
Review Comment:
Having `__str__` and `__repr__` return byte-identical JSON is redundant.
Python falls back to `__repr__` when `__str__` isn't defined, so you can drop
`__str__` entirely. If they ever need to diverge, they should diverge
meaningfully.
##########
task-sdk/src/airflow/sdk/definitions/param.py:
##########
@@ -359,6 +359,22 @@ def deserialize(cls, data: dict, dags: dict) -> DagParam:
return cls(current_dag=current_dag, name=data["name"],
default=data["default"])
+ def __repr__(self) -> str:
Review Comment:
Returning JSON from `__repr__` is unusual. `__repr__` is for humans
debugging (ideally eval-friendly, e.g. `DagParam(dag_id='d',
name='exponent')`), while serialization contracts live on `.serialize()`.
Mixing the two means every log line and error message that formats a `DagParam`
now shows JSON, which is noisier than the existing pattern (`MappedArgument`'s
attrs-generated repr, `XComArg.__repr__`).
If the goal is just a stable string, `@attrs.define` would give you that for
free and match surrounding code.
--
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]
