Handle DockerOperator mounts after rendering instead of in __init__#70342
Handle DockerOperator mounts after rendering instead of in __init__#703421fanwang wants to merge 5 commits into
Conversation
shahar1
left a comment
There was a problem hiding this comment.
Could you please resolve conflicts?
There was a problem hiding this comment.
AI Summary
Found one blocking regression: Mount instances passed by the user no longer survive template rendering — execute() raises TypeError before the container starts.
Posting this as a comment rather than a second change-request: my earlier review already has the PR gated, so there's no need to stack another. The substance below is still a merge blocker.
Mount inputs crash at execute() after rendering (providers/docker/src/airflow/providers/docker/operators/docker.py:490)
mounts is a template field, and the templater flattens any dict subclass — Mount included — into a plain dict:
# task-sdk/src/airflow/sdk/definitions/_internal/templater.py
if isinstance(value, dict):
return {k: self.render_template(v, context, jinja_env, oids) for k, v in value.items()}So a user-supplied Mount(source=..., target=..., type=...) reaches execute() as {'Target': ..., 'Source': ..., 'Type': ..., 'ReadOnly': False} — API-cased keys, and no longer a Mount. _normalize_mounts then takes the else branch and calls Mount(**m) with those keys, which Mount.__init__(target, source, type=...) does not accept:
TypeError: Mount.__init__() got an unexpected keyword argument 'Target'. Did you mean 'target'?
That is the primary documented usage, and the one the provider's own example Dag uses (providers/docker/tests/system/docker/example_docker_copy_data.py:76). Reproduced with the same test file and mocks on both branches:
| result | |
|---|---|
main (dc42801bde) |
passes — mounts reach create_host_config as [{'Target': '/test', 'Source': 'workspace', 'Type': 'volume', 'ReadOnly': False}] |
this PR (bcc3ae18) |
TypeError: Mount.__init__() got an unexpected keyword argument 'Target' |
The existing test_dict_mounts_are_templated already shows the flattening — it renders a Mount and asserts rendered.mounts[0]["Target"]. Rendering produced that API-cased plain dict all along; before this PR nothing tried to rebuild a Mount from it, and docker-py accepts the plain dict as-is.
Since a rendered Mount is already in the shape docker-py wants, it just needs to be left alone — see the suggestion inline. DockerSwarmOperator inherits the method, so the same fix covers it.
Missing regression coverage for the render → execute path (providers/docker/tests/unit/docker/operators/test_docker.py:879)
- For bug-fix PRs, flag if there is no regression test — a test that fails without the fix and passes with it.
- Flag any existing test modified to accommodate new behavior — this may indicate a behavioral regression rather than a genuine fix.
—
.github/instructions/code-review.instructions.md§ Quality Signals to Check
test_dict_mounts_are_normalized_to_mount_objects now calls op.execute(None) directly, skipping render_templates(). The Mount in the fixture is therefore still a Mount when _normalize_mounts runs, and the isinstance guard short-circuits. The entire point of this change is what happens after rendering, and no test crosses that boundary — which is why CI is green on a broken path.
Something along these lines would have caught it:
@pytest.mark.db_test
def test_mount_objects_survive_render_then_execute(self, create_task_instance_of_operator):
ti = create_task_instance_of_operator(
operator_class=DockerOperator,
dag_id="test", task_id="test", image="test", mount_tmp_dir=False,
mounts=[Mount(source="workspace", target="/{{task_instance.run_id}}", type="volume")],
)
task = ti.render_templates()
task.execute(None)
passed = self.client_mock.create_host_config.call_args.kwargs["mounts"]
assert passed[0]["Target"] == f"/{ti.run_id}"Smaller observations
providers/docker/src/airflow/providers/docker/operators/docker.py:158— themountsdocstring still saysdictentries "are converted toMountinstances at construction time", which this PR makes untrue. Worth saying the conversion happens at execution time —op.mountsnow holds the raw input until then, which is a visible change for anything introspecting the operator.
This review was drafted by an AI-assisted tool and confirmed by an Apache Airflow maintainer. The findings below are observations, not blockers; an Apache Airflow maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting
mounts is a template field, rendered after __init__ runs. The constructor converted dict mounts into Mount objects and tagged each with template_fields so nested values would render. Mount is a dict subclass, so Jinja already renders its values natively without that tagging. Keep the raw input in __init__ and convert to Mount objects at the start of execute(), after rendering. related: apache#70296 Signed-off-by: 1fanwang <1fannnw@gmail.com>
Reduce the multi-line narration around deferring Mount conversion to a single concise note at each site. Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
DockerSwarmOperator overrides execute() and calls _run_service() without super().execute(), so the mount conversion moved out of __init__ never ran for it. Share the conversion via _normalize_mounts() and call it from both execute paths. Signed-off-by: 1fanwang <1fannnw@gmail.com>
A user-supplied Mount is a dict subclass, so the templater flattens it to a
plain API-cased dict ({Target, Source, Type, ReadOnly}) during rendering.
_normalize_mounts then hit the else branch and called Mount(**m) with those
keys, raising TypeError at execute() before the container started. A rendered
Mount is already the shape docker-py wants, so leave it (and any real Mount)
alone; only rebuild a Mount from a raw user dict.
Add a render-then-execute regression test that fails on the pre-fix source.
Signed-off-by: 1fanwang <1fannnw@gmail.com>
bcc3ae1 to
4ae16f0
Compare
shahar1
left a comment
There was a problem hiding this comment.
LGTM, could you please fix the AI attribution section in the PR body? (same goes to past PRs)
Done, it was messed up somehow during edits, fixed for this and previous PRs |
mountsis a template field, so it is rendered after__init__runs. The constructor converted dict mounts intoMountobjects and tagged each withtemplate_fields = ("Source", "Target", "Type")so their nested values would render — the template-field-in-__init__logic #70296 is burning down.Keep the raw input in
__init__and handle it at the start ofexecute(), after rendering. The SDK templater flattens anydictsubclass —Mountincluded — into a plain API-cased dict, so a rendered mount already has the shape docker-py wants: rebuild aMountonly from a raw user dict, and leave rendered dicts (and any realMount) alone.DockerSwarmOperatorshares_normalize_mounts, so it is covered by the same method.related: #70296
Testing Done
Ran the real render then execute path (docker client mocked), red on the pre-fix source and green after — the path a plain
execute()-only test misses, because the templater only flattens theMountduringrender_templates():test_mount_objects_survive_render_then_executebuilds a task instance with a templatedMount, callsrender_templates()thenexecute(), and asserts the mount reachingcreate_host_configrendered to/{run_id}. It fails on the pre-fix source and passes after.validate_operators_initno longer exemptsDockerOperator.Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI following the guidelines