naruto-lgtm commented on PR #68662: URL: https://github.com/apache/airflow/pull/68662#issuecomment-4882650005
Pushed the removal in eef021eb. I went with removing it outright rather than reintroducing a narrow allowlist. From the call-site sweep, once the cross-boundary control-flow signals are accounted for (they already travel as their own comms messages and get reconstructed explicitly in the task runner), nothing in the runtime depends on getting an exception instance back out of general serialization. So the AIRFLOW_EXC_SER/BASE_EXC_SER branches were only ever catching exceptions nested inside some other serialized value, which is exactly the general-case behaviour you wanted gone from a module that runs in so many processes. What changed: - Dropped both encode branches (the AirflowException-with-serialize one and the KeyError/AttributeError one) and the matching decode branch. No process running general serialization can import-and-call a payload name anymore; an exception now just serializes to its string form via the existing fallback. - Removed the two now-dead DagAttributeTypes members. - One thing the removal surfaced: the guard test (test_recursive_serialize_calls_must_forward_kwargs) was short-circuiting on the var.serialize() call in the exception branch, which had been masking a cls.serialize(list(var)) recursion for LazySelectSequence that dropped strict. I forwarded strict there so the invariant actually holds now. I didn't add the allowlisted helper since I couldn't find a case that needs one. If you know of a round-trip I've missed, point me at it and I'll add the targeted map, but my read is nothing outside the comms path relies on it. -- 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]
