Conversation
uranusjr
force-pushed
the
asset-event-extra-template
branch
from
432cb5c to
ef4e7b5
Compare
amoghrajesh
left a comment
amoghrajesh
left a comment
There was a problem hiding this comment.
The qn posted doesn't have much to do with the PR but a general observation, PR looks good
| # TODO: Deprecated in SDK. Remove in Airflow 4.0. | ||
| extra: dict[str, JsonValue] |
There was a problem hiding this comment.
| # TODO: Deprecated in SDK. Remove in Airflow 4.0. | |
| extra: dict[str, JsonValue] | |
| # TODO: Deprecated in SDK in 3.2. Remove in Airflow 4.0. | |
| extra: dict[str, JsonValue] |
| ) | ||
| for watcher in watchers | ||
| ], | ||
| # TODO: Deprecated in SDK. Remove in Airflow 4.0. |
There was a problem hiding this comment.
| # TODO: Deprecated in SDK. Remove in Airflow 4.0. | |
| # TODO: Deprecated in SDK in 3.2. Remove in Airflow 4.0. |
There was a problem hiding this comment.
I was thinking this might be confusing if we decouple SDK and Core versioning.
| @@ -404,6 +407,20 @@ def normalized_uri(self) -> str | None: | |||
| except ValueError: | |||
| return None | |||
|
|
|||
| @property | |||
| def extra(self) -> dict[str, JsonValue]: | |||
| # No warning here; a warning is shown when the value is set. | |||
| return self._extra | |||
|
|
|||
| @extra.setter | |||
| def extra(self, value: dict[str, JsonValue]) -> None: | |||
| warnings.warn( | |||
| "Asset.extra is deprecated and will be removed in the future.", | |||
| RemovedInAirflow4Warning, | |||
| stacklevel=2, | |||
| ) | |||
| self._extra = value | |||
There was a problem hiding this comment.
No complaint but we use RemovedInAirflow4Warning and DeprecatedImportWarning randomly, so just flagging that here if that one's more relevant, but in this case since we know we want to nuke in AF4, I think we are OK?
There was a problem hiding this comment.
Honestly I am not even sure when DeprecatedImportWarning should be used (I guess on imports…? And it also still implies removal on Airflow 4…?) But this is not supposed to be triggered on an import anyway so RemovedInAirflow4Warning is definitely the right one to use.
I actually also had event_extra_template implemented locally, but when I was about to write the documentation, I realised its use case is… very limited? It can only be static, and really the worst case scenario without it is you repeat them when you emit events.
Since we generally discourage emitting events of the same asset in different dags anyway, all this really helps would be to save you a few keystrokes. I couldn’t justify to myself to implement it.
Therefore, this PR only contains deprecation to a confusing and not-really-useful feature. It does not provide a replacement.
Close #55200 (without implementing everything it requests—see explaination above)