Skip to content

[AIRFLOW-5138] Don't override user's warning settings in airflow.configuration#5756

Closed
jml wants to merge 1 commit intoapache:masterfrom
jml:airflow-5138-spurious-warnings
Closed

[AIRFLOW-5138] Don't override user's warning settings in airflow.configuration#5756
jml wants to merge 1 commit intoapache:masterfrom
jml:airflow-5138-spurious-warnings

Conversation

@jml
Copy link
Contributor

@jml jml commented Aug 8, 2019

Jira

Partly addresses https://issues.apache.org/jira/browse/AIRFLOW-5138 by allowing users to silence the spurious warnings.

It's very uncommon for Python libraries to override warning settings in this way. By doing so, tools like pytest filterwarnings or even python -W won't suppress the warnings, which means the warnings just become background noise, rather than a meaningful message from one person to another.

Tests

My PR doesn't need automated tests because it's reverting warnings behaviour to the Python default, which is fine for practically everyone else.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

None required. No behaviour changes.

Code Quality

  • Passes flake8 — your CI doesn't catch this?!

Overriding warnings settings means that users cannot choose how to process
these warnings. This means that in the case of spurious warnings, users are
left with no choice but to see them over and over, which can blind them to
warnings that really matter.
@ashb
Copy link
Member

ashb commented Aug 19, 2019

The problem with not setting this is that by default (since Py3) DeprecationWarning will be silenced, so with this change users would never see Airflow deprecation warnings.

@ashb
Copy link
Member

ashb commented Aug 19, 2019

Passes flake8 — your CI doesn't catch this?!

It does, but we'd rather people checked it themselves before hand. It's also a good indication of when people do or don't read 😉

@jml
Copy link
Contributor Author

jml commented Aug 19, 2019

The problem with not setting this is that by default (since Py3) DeprecationWarning will be silenced, so with this change users would never see Airflow deprecation warnings.

I see.

Since these are warnings are intended for users of the airflow executable, rather than people calling Airflow APIs, they should probably be FutureWarnings (as per https://www.python.org/dev/peps/pep-0565/#additional-use-case-for-futurewarning).

Alternatively, the warning should be enabled in the script, not in the configuration module.

@jml
Copy link
Contributor Author

jml commented Aug 20, 2019

I don't have the capacity to pursue this further. Maybe I'll try again later.

@jml jml closed this Aug 20, 2019
@ashb
Copy link
Member

ashb commented Aug 22, 2019

"FutureWarning" is proabably right for some of the use cases here (wrong config file, etc) but there are other cases where Airflow dual life as both an application and a library butt heads: Some hooks have deprecated some of their methods - should those be DeprecationWarning or FutureWarning?

A "quick" fix might be an env var such as AIRFLOW_NO_DEPRECATION_WARNINGS that would leave the filters as default?

@jml
Copy link
Contributor Author

jml commented Aug 23, 2019

Hooks with deprecated methods should use DeprecationWarning, and Airflow should leave it up to the user (i.e. the Python developers calling those hooks) to configure their DeprecationWarning levels.

Personally, I'd rather fix the false warnings than add an environment variable. Adding yet another configuration control and yet another if just adds to the complexity, and I think Airflow probably needs less of that, not more.

@ashb
Copy link
Member

ashb commented Aug 23, 2019

Unfortunately most of our users aren't traditional python developers, so if we don't enable deprecation warnings (or use FutureWarning) for cases like that then many users would never see the warning

@max-sixty
Copy link

Unfortunately most of our users aren't traditional python developers, so if we don't enable deprecation warnings (or use FutureWarning) for cases like that then many users would never see the warning

Why not just use FutureWarnings then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants