-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Respect default_args in DAG when its set to a "falsy" value #57853
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
Conversation
|
The ser dag tests are failing, as we've removed some default falsy values. I think that's okay, but we need to examine those closely. |
airflow-core/tests/unit/serialization/test_dag_serialization.py
Outdated
Show resolved
Hide resolved
|
👁️ looking at the failures |
|
@ashb this one wasn't as easy as I thought. The PR is updated now, can you take a look? |
|
rebasing for triggering the test |
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
|
@Lee-W it worked haha! |
Lee-W
left a comment
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.
Nice! Then others are nits from my end.
ashb
left a comment
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.
Is it worth also checking out works
airflow-core/tests/unit/serialization/test_dag_serialization.py
Outdated
Show resolved
Hide resolved
ephraimbuddy
left a comment
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.
Looks good. My only issue is on performance
…ue (apache#57853) (cherry picked from commit c03fc79) Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
closes: #57792
Problem
When
retries=0was explicitly set in DAGdefault_args, the value was being ignored during serialization, causing tasks to fall back to thedefault_task_retriesconfiguration value instead of the explicitly set0.Example
Cause
The serialization logic excluded values that matched schema defaults (e.g.,
retries=0) from the serialized json even when they differed fromclient_defaults(e.g.,retries=3from config). During deser, whenretrieswas missing from the serialized task data it would result it to back toclient_defaults.tasks.retries(3), losing the explicitly set value (0).Fix
Updated the ser/deser logic to only exclude values that match both the schema default AND
client_defaults. If a value matches the schema default but differs fromclient_defaults, it is included because that means it was explicitly set.Additionally, added a generic method
get_operator_const_fields()to detect fields with"const": truein the schema (like_is_sensorand_is_mapped), which should always be excluded whenFalse, regardless ofclient_defaults.Testing
Before:
After:
Tested with DAG:
Result with no retries:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.