Suppress import errors for providers from sources by dstandish · Pull Request #22579 · apache/airflow

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish · 2022-03-28T22:20:18Z

When we are running airflow locally with providers installed from sources, often many providers will be discovered which we haven't installed the deps for. This generally results in a very large amount of traceback logging, which has a very negative effect on usefulness of terminal output. Here we suppress this error logging for providers that are installed from sources.

potiuk

Looks cool! Thanks!

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

github-actions · 2022-03-28T22:44:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2022-03-28T22:46:10Z

Also 'airflow providers list' cmd needs fixing too :)

potiuk · 2022-03-28T22:48:42Z

(Remnant from time when ProviderInfo was just a tuple it seems).

dstandish · 2022-03-29T02:15:32Z

NIT: I'd use Enum or bool for providers/sources, but having string is nice for debugging too, so no problem wiht it (and TIL about post_init_)

yeah thought about Enum but thought it was too heavy handed for this

But bool is a good idea then we don't need post_init

PR apache#22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

PR #22579 suppressed provider import log warnings when running from sources. For whatever reason, CI didn't run test_providers_manager.py so I didn't catch that changes were needed there.

dstandish requested a review from potiuk March 28, 2022 22:30

potiuk approved these changes Mar 28, 2022
View reviewed changes

github-actionsBot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 28, 2022

uranusjr reviewed Mar 29, 2022
View reviewed changes

Comment threadairflow/providers_manager.py Outdated

dstandish added 2 commits March 28, 2022 20:52

fix typing and tuple access
8610f7e

fix references; rename prov info to data
a00b76a

dstandish requested review from ephraimbuddy and mik-laj as code owners March 29, 2022 05:02

fix test mock
b863807

dstandish merged commit b5a786b into apache:mainMar 29, 2022

dstandish deleted the providers-from-sources-import-error-to-debug branch March 29, 2022 15:56

dstandish mentioned this pull request Mar 29, 2022
Reduce verbosity of provider import failure logs & make configurable #22539
Closed

dstandish mentioned this pull request Mar 29, 2022
Fix providers manager tests #22592
Merged

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022