Be verbose about failure to import airflow_local_settings #17195

baryluk · 2021-07-23T16:48:29Z

Currently, if the module exists, but has errors (for example syntax
error, or transitive import of module that does not exist),
the airflow scheduler will not show any error.

In my opinion ImportError of airflow_local_settings should
be a fatal error, but let it be a warning for now.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

boring-cyborg · 2021-07-23T16:48:31Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

potiuk · 2021-07-23T16:53:39Z

I also think it should be a fatal error. Anyone thinks otherwise?

baryluk · 2021-07-23T16:55:03Z

Actually transitive imports might still be not handled correctly.

Let me address this by checking the ModuleNotFoundError.name field

potiuk · 2021-07-23T16:56:20Z

Yeah. exactly - failure to import local_settings at all is OK. but if the import inside local_settings fails, this should be a fatal error.

airflow/settings.py

potiuk

Agree we should raise error but raise is better as it keeps location of the original exception.

airflow/settings.py

potiuk

Nice! Really useful to detect some hard-to-debug problems!

github-actions · 2021-07-23T17:26:27Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

baryluk · 2021-07-23T17:29:03Z

Nice! Really useful to detect some hard-to-debug problems!

It did bite me of course, that is why I am suggesting this change. I replaced from airflow.contrib.kubernetes.pod import Pod with from airflow.kubernetes.pod import Pod blindly. And was surirpised that some spark kubernetes jobs could not find secrets that were provided in the pod_mutation_hook in the airflow_local_settings.py, with no logging of the issue at all. :D

potiuk · 2021-07-25T16:00:28Z

Closed/reopened to re-trigger the build

potiuk · 2021-07-25T18:35:21Z

Some static checks and tests are failing @baryluk

baryluk · 2021-07-26T12:56:39Z

Some static checks and tests are failing @baryluk

@potiuk should be fixed. can rerun CI.

potiuk · 2021-07-26T14:15:09Z

We had main broken so the test failures are unrelated. Those main errors were fixed this morning - can you please rebase to latest main ?

Currently, if the module exists, but has errors (for example syntax error, or transitive import of module that does not exist), the airflow scheduler will not show any error. In my opinion `ImportError` of `airflow_local_settings` should be a fatal error, but let it be a warning for now.

A case of `airflow_local_settings.py` importing a module that does not exist, will also throw `ModuleNotFoundError`, but it should not be silently ignored.

`ModuleNotFoundError` is a subclass of `ImportError`, so must be handled first.

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

baryluk · 2021-07-26T15:12:29Z

We had main broken so the test failures are unrelated. Those main errors were fixed this morning - can you please rebase to latest main ?

Rebased.

potiuk · 2021-07-26T15:46:18Z

Still static check problems. I highly recommend installing pre-commit, it will fix those kind of problems for you automatically :)

potiuk · 2021-07-26T16:01:46Z

And tests are still failing (this time settings tests, so you will need to fix it)

Makes black happy

potiuk · 2021-07-26T18:38:03Z

seems like transient errors only. Merging.

boring-cyborg · 2021-07-26T18:38:53Z

Awesome work, congrats on your first merged pull request!

baryluk · 2021-07-26T18:40:02Z

@potiuk Thanks for your patience and quick review!

* Be verbose about failure to import airflow_local_settings Currently, if the module exists, but has errors (for example syntax error, or transitive import of module that does not exist), the airflow scheduler will not show any error. A case of `airflow_local_settings.py` importing a module that does not exist, will also throw `ModuleNotFoundError`, but it should not be silently ignored.

potiuk requested changes Jul 23, 2021

View reviewed changes

airflow/settings.py Show resolved Hide resolved

potiuk requested changes Jul 23, 2021

View reviewed changes

airflow/settings.py Outdated Show resolved Hide resolved

airflow/settings.py Outdated Show resolved Hide resolved

potiuk approved these changes Jul 23, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jul 23, 2021

potiuk closed this Jul 25, 2021

potiuk reopened this Jul 25, 2021

baryluk and others added 6 commits July 26, 2021 17:11

Do not ignore transitive import failures from airflow_local_settings

a1cc089

A case of `airflow_local_settings.py` importing a module that does not exist, will also throw `ModuleNotFoundError`, but it should not be silently ignored.

Reorder ModuleNotFoundError, make it critical

ffe6dc7

`ModuleNotFoundError` is a subclass of `ImportError`, so must be handled first.

Use raise to reraise

72e03de

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

Use raise to reraise

b4485d2

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

Remove unused variable name in except

d3b1d8d

baryluk force-pushed the patch-1 branch from 377a3e9 to d3b1d8d Compare July 26, 2021 15:11

baryluk added 2 commits July 26, 2021 18:49

Reformat long function invocation

abe9253

Makes black happy

Update test_settings.py

b326747

potiuk merged commit 9504cb5 into apache:main Jul 26, 2021

baryluk deleted the patch-1 branch July 26, 2021 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be verbose about failure to import airflow_local_settings #17195

Be verbose about failure to import airflow_local_settings #17195

baryluk commented Jul 23, 2021

boring-cyborg bot commented Jul 23, 2021

potiuk commented Jul 23, 2021 •

edited

baryluk commented Jul 23, 2021

potiuk commented Jul 23, 2021 •

edited

potiuk left a comment

potiuk left a comment

github-actions bot commented Jul 23, 2021

baryluk commented Jul 23, 2021

potiuk commented Jul 25, 2021

potiuk commented Jul 25, 2021

baryluk commented Jul 26, 2021

potiuk commented Jul 26, 2021

baryluk commented Jul 26, 2021

potiuk commented Jul 26, 2021

potiuk commented Jul 26, 2021

potiuk commented Jul 26, 2021

boring-cyborg bot commented Jul 26, 2021

baryluk commented Jul 26, 2021

Be verbose about failure to import airflow_local_settings #17195

Be verbose about failure to import airflow_local_settings #17195

Conversation

baryluk commented Jul 23, 2021

boring-cyborg bot commented Jul 23, 2021

potiuk commented Jul 23, 2021 • edited

baryluk commented Jul 23, 2021

potiuk commented Jul 23, 2021 • edited

potiuk left a comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2021

baryluk commented Jul 23, 2021

potiuk commented Jul 25, 2021

potiuk commented Jul 25, 2021

baryluk commented Jul 26, 2021

potiuk commented Jul 26, 2021

baryluk commented Jul 26, 2021

potiuk commented Jul 26, 2021

potiuk commented Jul 26, 2021

potiuk commented Jul 26, 2021

boring-cyborg bot commented Jul 26, 2021

baryluk commented Jul 26, 2021

potiuk commented Jul 23, 2021 •

edited

potiuk commented Jul 23, 2021 •

edited