Repository navigation
Conversation
uranusjr
requested review from
XD-DENG,
ashb,
bbovenzi,
kaxil and
ryanahamilton
as code owners
March 21, 2022 10:49
This comment was marked as outdated.
This comment was marked as outdated.
This is needed for the /rendered-templates view when if a DAG is never run.
uranusjr
force-pushed
the
mapped-operator-template-fields-renderers-2
branch
from
March 24, 2022 05:32
b43a9ed to
3a84de9
Compare
To support viewing an already-run mapped task in the web UI, we need to be able to unmap a serialized mapped operator. This allows unmapping the operator minimally to a SerializedBaseOperator, which should be enough for UI purposes.
Since an unmapped task is only used for template rendering, it does not need to modify its parent DAG (and task group) hierarchy. In fact, this in-place modification is arguably wrong in the first place, since a non-mapped task is always copied for execution, so the one being executed is never the one in the DAG, but another task that's identical to it. The task replacement is also harmful for things that do not intend to modify the DAG, such as the /rendered-templates view; modifying the DAG would corrupt it and cause subsequent accesses to not able to retrieve the correct mapping information.
uranusjr
force-pushed
the
mapped-operator-template-fields-renderers-2
branch
from
March 24, 2022 10:20
3a84de9 to
0a5744c
Compare
ashb
reviewed
Mar 29, 2022
This original task is needed afterwards to calculate dependants. Since we no longer add the rendered (unmapped) task to the DAG, the task does not contain correct task hierarchy information.
uranusjr
force-pushed
the
mapped-operator-template-fields-renderers-2
branch
from
March 29, 2022 10:14
c12a89d to
fef7d91
Compare
ashb
approved these changes
Mar 30, 2022
Contributor
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are a few things that needed fixing:
template_fields_renderersneeded to be serialised for MappedOperator so the web view can render the fields correctly.render_templates()to always be called byTaskInstance.get_rendered_template_fields(). This is now fixed.unmap()needs to support serialised operators. This is conceptually impossible, but can be approximated by unmapping into aSerializedBaseOperatorand copy all the attributes fromMappedOperatorto it. This is good enough for cases where task instance fields are pre-rendered, sinceRenderedTaskInstanceFieldswould be able to re-populate the real attributes; for fields not yet rendered (i.e. from a DAG that’s never run),/rendered-templateshas always been inaccurate, so it’s not that big a deal to not able to correctly populate all the fields (and it’s impossible anyway).unmap()is that we need to get rid of its side effect of replacing the mapped task with an unmapped one in the DAG, because the webserver won’t be able to recover from this replacement and would lose all operator-mapping information if we do that. Fortunately, I believe the task replacement is not needed in the first place, and can be avoided entirely without consequences. Furthermore, we already have a mechanism for unmapping a task without adding it to the DAG (the task validation API); we just need to rename the argument and attribute to reflect its broader usage.Note the clicking on the Rendered button still does not work after this PR, because the button is currently missing a correct
map_indexargument. We need to tweak the modal to be able to display a mapped task correctly. cc @bbovenzi