-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Handle default values from Airflow 2 gracefully #47371
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
10a4da2 to
7d539a4
Compare
f8dd35a to
9ff5266
Compare
d00db25 to
69b7eb1
Compare
69b7eb1 to
5aba5aa
Compare
4cb04be to
4c438c4
Compare
02372a6 to
c03bac3
Compare
airflow/cli/cli_config.py
Outdated
| help="The section name", | ||
| ) | ||
| ARG_LINT_CONFIG_UPGRADE_DEFAULTS = Arg( | ||
| ("--upgrade-defaults",), |
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.
| ("--upgrade-defaults",), | |
| ("--upgrade-problematic-defaults",), |
It's probably worth renaming these flags to include problematic - without it, it implies it'll upgrade all of the old defaults (which isn't a bad idea!).
Probably also means the Arg variable name should change too.
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.
This only “upgrades” them in the current process, right? As far as I can tell the new values are not persisted. I wonder if there is a better verb to describe this.
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.
@uranusjr any suggestion on prefix i could add to the name?
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.
Not sure. Maybe --patch-problematic-defaults instead?
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.
So, now that you bring it up, not sure this flag is even worth having. Upgrading it or not in the config lint command process doesn't seem that helpful?
But wait! In configuration.py you are already upgrading to the new default, meaning by the time config lint runs, well, it's already run and updated stuff. So having this run in config lint at all isn't doing anything really.
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.
I can maybe have upgrade=False in configuration.py, which wouldn't upgrade the confs by default. Is this what we intend to do? Or do we always want to update in any case?
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.
We do want upgrading by default, these are known bad values. I was just pointing out that doing thigns in config lint would never find/warn/upgrade anything - it's already too late.
df0c37f to
ae65d26
Compare
3cd1b18 to
c39acba
Compare
| ("core", "dag_ignore_file_syntax"): ( | ||
| re.compile(r"^regexp$"), | ||
| "3.0", | ||
| "The default value changed from 'regexp' in Airflow 2.x to 'glob' in Airflow 3.0.", | ||
| ), |
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.
| ("core", "dag_ignore_file_syntax"): ( | |
| re.compile(r"^regexp$"), | |
| "3.0", | |
| "The default value changed from 'regexp' in Airflow 2.x to 'glob' in Airflow 3.0.", | |
| ), |
Looking at this, automatically changing folks away from the default may break things, not using an old default. e.g. if their airflowignore was regex, but we automatically swap to glob, things won't be happy.
We should leave this one out I believe.
airflow/cli/cli_config.py
Outdated
| help="The section name", | ||
| ) | ||
| ARG_LINT_CONFIG_UPGRADE_DEFAULTS = Arg( | ||
| ("--upgrade-defaults",), |
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.
So, now that you bring it up, not sure this flag is even worth having. Upgrading it or not in the config lint command process doesn't seem that helpful?
But wait! In configuration.py you are already upgrading to the new default, meaning by the time config lint runs, well, it's already run and updated stuff. So having this run in config lint at all isn't doing anything really.
| if not args.skip_problematic_default_checks: | ||
| conf.handle_incompatible_airflow2_defaults(upgrade=args.upgrade_problematic_defaults) | ||
|
|
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.
Which means, since this has already run and upgraded, you can get rid of this completely - and the args, etc. All of it.
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.
It is unfortunate that this is what you get:
$ airflow config lint
/Users/jedc/github/airflow/airflow/configuration.py:440 FutureWarning: [core] dag_ignore_file_syntax: The default value changed from 'regexp' in Airflow 2.x to 'glob' in Airflow 3.0. Auto-upgraded from 'regexp' to 'glob'.
No issues found in your airflow.cfg. It is ready for Airflow 3!
The warning is from configuration.py, where it got auto-upgraded.
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.
This was a behaviour that I intentionally added where it automatically upgraded by default. If we want to change this to upgrade only via airflow config lint, I can pass upgrade=False in configuration.py method.
| assert expected_message in normalized_output | ||
| assert config_change.suggestion in normalized_output | ||
|
|
||
| def test_lint_detects_default_value_change_upgrade(self): |
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.
Also means this test should be refactored and moved into test_configuration.py.
|
|
||
| legacy_incompatible_defaults: dict[tuple[str, str], tuple[Pattern, str, str]] = { | ||
| ("logging", "log_filename_template"): ( | ||
| re.compile( |
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.
fwiw, I took the default out of a fresh 2.10.5 config file, and this regex did not match against it. The regex may have been cut off at the end of the task_id line (that's naively what printed, at least).
Give that a test.
|
Looking closer, this duplicates the functionality of deprecated_values. We can just update that. I've opened #47761 to do that. |
|
Closing this as discussed with @jedcunningham as per #47371 (comment) |
AirflowConfigParserclass calledhandle_incompatible_airflow2_defaultsfor this. It runs by default while loading config.airflow config lintAutomatically upgrade problematic default values:
airflow config lint --upgrade-problematic-defaultsSkip checking default values:
airflow config lint --skip-problematic-default-checksScreenshot of testing done locally:
airflow config lintand importingconfin shellBelow is a summary table of the changes:
closes: #46972
^ 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 newsfragments.