Repository navigation
Fix task config defaults for language SDKs #75267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2153,11 +2153,11 @@ def from_dict(cls, serialized_obj: dict) -> SerializedDAG: | |||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||
| def fill_config_defaults(cls, serialized_obj: dict[str, Any]) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| Fill in the Dag settings a serialized Dag leaves unset from the Airflow config, as a Python Dag does. | ||||||||||||||||||||||||||||||||||||||||||||||
| Fill config-backed defaults for a serialized Lang-SDK Dag, as a Python Dag does. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| A Lang-SDK runtime cannot read the Airflow config, so it leaves ``max_active_tasks``, | ||||||||||||||||||||||||||||||||||||||||||||||
| ``max_active_runs``, ``max_consecutive_failed_dag_runs``, ``catchup`` and | ||||||||||||||||||||||||||||||||||||||||||||||
| ``disable_bundle_versioning`` out unless the Dag sets them. A value the Dag sets is kept. | ||||||||||||||||||||||||||||||||||||||||||||||
| A Lang-SDK runtime cannot read the Airflow config, so it leaves config-backed Dag settings out | ||||||||||||||||||||||||||||||||||||||||||||||
| unless the Dag sets them. Airflow fills those settings and supplies config-backed task defaults | ||||||||||||||||||||||||||||||||||||||||||||||
| through ``client_defaults.tasks``, without replacing task fields the SDK explicitly serialized. | ||||||||||||||||||||||||||||||||||||||||||||||
| *serialized_obj* is changed in place. | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| dag = serialized_obj.get("dag") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2179,6 +2179,32 @@ def fill_config_defaults(cls, serialized_obj: dict[str, Any]) -> None: | |||||||||||||||||||||||||||||||||||||||||||||
| if key not in dag: | ||||||||||||||||||||||||||||||||||||||||||||||
| dag[key] = get(section, option) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| # Native runtimes cannot read Airflow's task defaults. Supply only the config-backed | ||||||||||||||||||||||||||||||||||||||||||||||
| # fields the Python serializer exposes through client_defaults, so explicit values in | ||||||||||||||||||||||||||||||||||||||||||||||
| # the task itself (including values equal to schema defaults) retain precedence. | ||||||||||||||||||||||||||||||||||||||||||||||
| task_default_keys = { | ||||||||||||||||||||||||||||||||||||||||||||||
| "owner", | ||||||||||||||||||||||||||||||||||||||||||||||
| "queue", | ||||||||||||||||||||||||||||||||||||||||||||||
| "retries", | ||||||||||||||||||||||||||||||||||||||||||||||
| "retry_delay", | ||||||||||||||||||||||||||||||||||||||||||||||
| "weight_rule", | ||||||||||||||||||||||||||||||||||||||||||||||
| "execution_timeout", | ||||||||||||||||||||||||||||||||||||||||||||||
| "email_on_failure", | ||||||||||||||||||||||||||||||||||||||||||||||
| "email_on_retry", | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| task_defaults = { | ||||||||||||||||||||||||||||||||||||||||||||||
| key: value | ||||||||||||||||||||||||||||||||||||||||||||||
| for key, value in OperatorSerialization.generate_client_defaults().items() | ||||||||||||||||||||||||||||||||||||||||||||||
| if key in task_default_keys | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2182
to
+2199
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whitelist is a second copy of knowledge
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
| if task_defaults: | ||||||||||||||||||||||||||||||||||||||||||||||
| client_defaults = serialized_obj.setdefault("client_defaults", {}) | ||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(client_defaults, dict): | ||||||||||||||||||||||||||||||||||||||||||||||
| existing_task_defaults = client_defaults.setdefault("tasks", {}) | ||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(existing_task_defaults, dict): | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2200
to
+2204
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need the |
||||||||||||||||||||||||||||||||||||||||||||||
| for key, value in task_defaults.items(): | ||||||||||||||||||||||||||||||||||||||||||||||
| existing_task_defaults.setdefault(key, value) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||
| def validate_serialized_dag(cls, serialized_obj: dict[str, Any]) -> SerializedDAG: | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5466,6 +5466,77 @@ def test_keeps_a_setting_the_dag_sets(self): | |
|
|
||
| assert data == before | ||
|
|
||
| def test_fills_unset_task_defaults_from_config(self, monkeypatch): | ||
| task_defaults = { | ||
| "owner": "configured-owner", | ||
| "queue": "celery_q", | ||
| "retries": 3, | ||
| "retry_delay": 240.0, | ||
| "weight_rule": "upstream", | ||
| "execution_timeout": 600.0, | ||
| "email_on_failure": False, | ||
| "email_on_retry": True, | ||
| "pool": "default_pool", | ||
| "priority_weight": 10, | ||
| } | ||
| monkeypatch.setattr( | ||
| OperatorSerialization, | ||
| "generate_client_defaults", | ||
| classmethod(lambda _: task_defaults), | ||
| ) | ||
| task = {"__type": "operator", "__var": {"task_id": "explicit", "language": "typescript", "retries": 0}} | ||
| data = {"__version": 3, "dag": {"dag_id": "native_dag", "tasks": [task]}} | ||
|
|
||
| DagSerialization.fill_config_defaults(data) | ||
|
|
||
| assert data["client_defaults"] == { | ||
| "tasks": { | ||
| "owner": "configured-owner", | ||
| "queue": "celery_q", | ||
| "retries": 3, | ||
| "retry_delay": 240.0, | ||
| "weight_rule": "upstream", | ||
| "execution_timeout": 600.0, | ||
| "email_on_failure": False, | ||
| "email_on_retry": True, | ||
| } | ||
| } | ||
| assert data["dag"]["tasks"][0]["__var"]["retries"] == 0 | ||
|
|
||
| def test_merges_task_defaults_without_replacing_explicit_defaults(self, monkeypatch): | ||
| monkeypatch.setattr( | ||
| OperatorSerialization, | ||
| "generate_client_defaults", | ||
| classmethod(lambda _: {"retries": 3, "queue": "celery_q"}), | ||
| ) | ||
| data = { | ||
| "__version": 3, | ||
| "dag": {"dag_id": "native_dag", "tasks": [{"__type": "operator", "__var": {"task_id": "explicit", "language": "java"}}]}, | ||
| "client_defaults": {"tasks": {"retries": 1}}, | ||
| } | ||
|
|
||
| DagSerialization.fill_config_defaults(data) | ||
|
|
||
| assert data["client_defaults"] == {"tasks": {"retries": 1, "queue": "celery_q"}} | ||
|
|
||
| def test_fills_task_defaults_for_go_sdk_payloads_too(self, monkeypatch): | ||
| monkeypatch.setattr( | ||
| OperatorSerialization, | ||
| "generate_client_defaults", | ||
| classmethod(lambda _: {"retries": 3, "queue": "celery_q"}), | ||
| ) | ||
| data = { | ||
| "__version": 3, | ||
| "dag": { | ||
| "dag_id": "native_dag", | ||
| "tasks": [{"__type": "operator", "__var": {"task_id": "task", "language": "go"}}], | ||
| }, | ||
| } | ||
|
|
||
| DagSerialization.fill_config_defaults(data) | ||
|
|
||
| assert data["client_defaults"] == {"tasks": {"retries": 3, "queue": "celery_q"}} | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also add a new test that define a real Dag with some operators like in #74869 to exercise the set default behavior. |
||
| def test_leaves_a_payload_without_a_dag_to_validation(self): | ||
| data = {"__version": 3} | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -202,6 +202,8 @@ type specRules struct { | |||||||||||||||||||||
| // set names the lists that Python keeps in a set, so that Python writes the list sorted and | ||||||||||||||||||||||
| // without repeats. writeSpecFields writes it the same way. | ||||||||||||||||||||||
| set map[string]bool | ||||||||||||||||||||||
| // keepDefaults preserves an explicitly set task field when it equals the schema default. | ||||||||||||||||||||||
| keepDefaults bool | ||||||||||||||||||||||
|
Comment on lines
+205
to
+206
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A per-SDK flag for a rule that should hold everywhere. Dropping it here, in
Suggested change
|
||||||||||||||||||||||
| // skip names the fields that writeSpecFields never writes. The comment on each says why. | ||||||||||||||||||||||
| skip map[string]bool | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -218,7 +220,8 @@ var dagSpecRules = specRules{ | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var taskSpecRules = specRules{ | ||||||||||||||||||||||
| fields: taskSpecFields, | ||||||||||||||||||||||
| fields: taskSpecFields, | ||||||||||||||||||||||
| keepDefaults: true, | ||||||||||||||||||||||
|
Comment on lines
+223
to
+224
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| skip: map[string]bool{ | ||||||||||||||||||||||
| // Python writes email_on_failure and email_on_retry only for an operator that has an email | ||||||||||||||||||||||
| // recipient, and a TaskSpec has no field for a recipient. | ||||||||||||||||||||||
|
|
@@ -234,10 +237,11 @@ var taskSpecRules = specRules{ | |||||||||||||||||||||
| // from the defaults of Python's TaskGroup instead. | ||||||||||||||||||||||
| var taskGroupSpecRules = specRules{fields: taskGroupSpecFields} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // writeSpecFields writes into data each field of spec that rules does not skip. It leaves out a | ||||||||||||||||||||||
| // field that is unset or that holds its schema default. A field is unset when it holds its zero | ||||||||||||||||||||||
| // value. A pointer field is unset only when it is nil, so a pointer field can set the zero value of | ||||||||||||||||||||||
| // the type it points to, such as false or 0. | ||||||||||||||||||||||
| // writeSpecFields writes into data each field of spec that rules does not skip. It leaves out | ||||||||||||||||||||||
| // fields that are unset, and normally those that hold their schema default. Task rules preserve | ||||||||||||||||||||||
| // explicitly set schema-default values so Airflow config defaults cannot replace them. A field | ||||||||||||||||||||||
| // is unset when it holds its zero value. A pointer field is unset only when nil, so it can explicitly | ||||||||||||||||||||||
| // set the zero value of its type, such as false or 0. | ||||||||||||||||||||||
|
Comment on lines
+240
to
+244
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| func writeSpecFields(data map[string]any, spec any, rules specRules) { | ||||||||||||||||||||||
| value := reflect.ValueOf(spec) | ||||||||||||||||||||||
| for i := range value.NumField() { | ||||||||||||||||||||||
|
|
@@ -252,7 +256,7 @@ func writeSpecFields(data map[string]any, spec any, rules specRules) { | |||||||||||||||||||||
| continue | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| encoded, set := encodeSpecValue(value.Field(i), rules.set[name]) | ||||||||||||||||||||||
| if !set || (field.schemaDefault != nil && isSameJSON(encoded, field.schemaDefault)) { | ||||||||||||||||||||||
| if !set || (!rules.keepDefaults && field.schemaDefault != nil && isSameJSON(encoded, field.schemaDefault)) { | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| continue | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| data[field.key] = encoded | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -206,16 +206,17 @@ def compare_fields( | |||||||||||||
| """ | ||||||||||||||
| Compare two serialized objects key by key. | ||||||||||||||
|
|
||||||||||||||
| A key the SDK leaves out is fine when Python wrote its schema default. Python keeps such a value | ||||||||||||||
| when its ``client_defaults`` table disagrees with the schema, a table a language SDK is never sent, | ||||||||||||||
| and Airflow reads a missing field as its default anyway. | ||||||||||||||
| A key either side leaves out is fine when the value the other side writes equals the schema | ||||||||||||||
| default. Python may omit a value matching its ``client_defaults`` table, and native SDKs now preserve | ||||||||||||||
| explicitly set task values even when they equal the schema default. | ||||||||||||||
|
Comment on lines
+209
to
+211
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| """ | ||||||||||||||
| problems = [] | ||||||||||||||
| for key in sorted((python.keys() | sdk.keys()) - not_compared): | ||||||||||||||
| if key in python and key in sdk: | ||||||||||||||
| problems.extend(find_differences(key, python[key], sdk[key])) | ||||||||||||||
| elif key in sdk: | ||||||||||||||
| problems.append(f"{key} is {sdk[key]!r}, which Python does not write") | ||||||||||||||
| if key not in defaults or not is_same_json(sdk[key], defaults[key]): | ||||||||||||||
| problems.append(f"{key} is {sdk[key]!r}, which Python does not write") | ||||||||||||||
| elif key not in defaults or not is_same_json(python[key], defaults[key]): | ||||||||||||||
| problems.append(f"{key} is missing, Python writes {python[key]!r}") | ||||||||||||||
| return problems | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth stating as one rule rather than a Go exception.