You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kaxil (via GitHub)" <gi...@apache.org> on 2025/08/22 03:50:34 UTC
[PR] Remove ``unmap`` method from scheduler-side [airflow]
kaxil opened a new pull request, #54816:
URL: https://github.com/apache/airflow/pull/54816
The scheduler-side `MappedOperator` and `SerializedBaseOperator` classes contained `unmap()` functionality that was creating unnecessary complexity and architectural confusion. The `unmap()` method was attempting to synthesize "real" operators from serialized data, but this is not needed for scheduler operations.
Scheduler doesn't execute callbacks - that's handled by the DAG processor. So simplifying the codebase.
This would also be helpful for https://github.com/apache/airflow/pull/54569
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2293583342
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
`models.MappedOperator` also defines `def get_extra_links` -- so either is already broken or there should be no change with this 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2304217401
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
One extra wrinkle here @uranusjr is that plugon-based global operator links would still possible work in the Webserver.
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2296375668
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
Actually I think this was from a series of incorrect changes… get_link used to only accept BaseOperator before this change
https://github.com/apache/airflow/pull/46613/
You can see `get_extra_links` always calls `unmap` to get a BaseOperator.
After the PR above, `get_extra_links` was then incorrectly “restored” to pass in MappedOperator in #50238. This PR has not been a release yet.
I think removing `unmap` here is therefore wrong. It should be kept for `get_extra_links` for compatibility.
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2296382049
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
@uranusjr https://github.com/apache/airflow/pull/46613 is released in 3.0.0 and https://github.com/apache/airflow/pull/50238 in 3.0.1
>get_link used to only accept BaseOperator before this change
How would that work with `CustomOperators` though! We don't serialize all attributes so `extra_links` with `operator` as argument will have limited attributes to work with for global op links
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2293584703
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
https://github.com/apache/airflow/blob/e8219152cbfafc7fbc1c0b169ebc7bb1cb71d0d1/airflow-core/src/airflow/models/mappedoperator.py#L352-L367
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2296262872
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
With 3.0.0 get links aren't called in the scheduler or really the webserver anymore - we get the link in the execution side and store it in the xcom as an XComExtraLink (or something like 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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2302888036
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
When the thing was original designed we _could_ get the original operator class from a dag bag… but yeah I guess all these no longer apply now.
Now that we always do get_link in the worker (https://github.com/apache/airflow/pull/54816#discussion_r2296262872), we can always get the original operator class (you need to unmap for execution anyway), so I guess what we need to do is
1. _Not_ have `extra_links` and `get_extra_links` on SerializedBaseOperator? (since nobody should access these in the scheduler and webserver; the XCom mnechanism should be used instead)
1. Make sure global links are also called and generated on execution time
1. Restore get_link to only be expect BaseOperator subclasses; may need to tweak execution slightly to make sure it’s only called after a MappedOperator is unmapped into a BaseOperator
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2296263219
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
I guess this only gets called for plugin registered global links.
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #54816:
URL: https://github.com/apache/airflow/pull/54816
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2292810807
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
Hmm, I wonder if this should be kept. Would users expect get_link to be called against a MappedOperator? It may not have the same attributes as the underlying operator.
--
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
Re: [PR] Remove ``unmap`` method from scheduler-side [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #54816:
URL: https://github.com/apache/airflow/pull/54816#discussion_r2302888036
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -1309,7 +1308,7 @@ def get_extra_links(self, ti: TaskInstance, name: str) -> str | None:
link = self.operator_extra_link_dict.get(name) or self.global_operator_extra_link_dict.get(name)
if not link:
return None
- return link.get_link(self.unmap(None), ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
+ return link.get_link(self, ti_key=ti.key) # type: ignore[arg-type] # TODO: GH-52141 - BaseOperatorLink.get_link expects BaseOperator but receives SerializedBaseOperator
Review Comment:
When the thing was original designed we _could_ get the original operator class from a dag bag… but yeah I guess all these no longer apply now.
Now that we always do get_link in the worker (https://github.com/apache/airflow/pull/54816#discussion_r2296262872), we can always get the original operator class (you need to unmap for execution anyway), so I guess what we need to do is
1. This PR is OK, we don’t need unmap at scheduler side
1. _Not_ have `extra_links` and `get_extra_links` on SerializedBaseOperator? (since nobody should access these in the scheduler and webserver; the XCom mnechanism should be used instead)
1. Make sure global links are also called and generated on execution time
1. Restore get_link to only be expect BaseOperator subclasses; may need to tweak execution slightly to make sure it’s only called after a MappedOperator is unmapped into a BaseOperator
--
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