Skip to content

Fix /rendered-templates for mapped operator - #22396

Merged
uranusjr merged 6 commits into
apache:mainfrom
astronomer:mapped-operator-template-fields-renderers-2
Mar 30, 2022
Merged

uranusjr merged 6 commits into
apache:mainfrom
astronomer:mapped-operator-template-fields-renderers-2

Conversation

@uranusjr

@uranusjr uranusjr commented Mar 21, 2022 •

Copy link
Copy Markdown
Member

There are a few things that needed fixing:

  1. template_fields_renderers needed to be serialised for MappedOperator so the web view can render the fields correctly.
  2. A bug was accidentally introduced in Implement mapped value unpacking #21641 that caused render_templates() to always be called by TaskInstance.get_rendered_template_fields(). This is now fixed.
  3. To allow templated fields to be rendered on the correct operator instance (the unmapped one), unmap() needs to support serialised operators. This is conceptually impossible, but can be approximated by unmapping into a SerializedBaseOperator and copy all the attributes from MappedOperator to it. This is good enough for cases where task instance fields are pre-rendered, since RenderedTaskInstanceFields would be able to re-populate the real attributes; for fields not yet rendered (i.e. from a DAG that’s never run), /rendered-templates has 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).
  4. Another consequence on supporting 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_index argument. We need to tweak the modal to be able to display a mapped task correctly. cc @bbovenzi

@uranusjr

This comment was marked as outdated.

@uranusjr uranusjr changed the title Add template_fields_renderers to MappedOperator Fix /rendered-templates for mapped operator Mar 23, 2022
This is needed for the /rendered-templates view when if a DAG is never
run.
@uranusjr
uranusjr force-pushed the mapped-operator-template-fields-renderers-2 branch from b43a9ed to 3a84de9 Compare March 24, 2022 05:32
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
uranusjr force-pushed the mapped-operator-template-fields-renderers-2 branch from 3a84de9 to 0a5744c Compare March 24, 2022 10:20
Comment thread airflow/models/taskinstance.py Resolved
Comment thread airflow/models/mappedoperator.py Resolved
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
uranusjr force-pushed the mapped-operator-template-fields-renderers-2 branch from c12a89d to fef7d91 Compare March 29, 2022 10:14
@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 30, 2022
@github-actions

Copy link
Copy Markdown
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.

@uranusjr
uranusjr merged commit 143702e into apache:main Mar 30, 2022
@uranusjr
uranusjr deleted the mapped-operator-template-fields-renderers-2 branch March 30, 2022 16:36
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dynamic-task-mapping AIP-42 area:serialization area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants