[AIRFLOW-6005] Common classes should always be imported from airflow, Depends on [AIRFLOW-6128]

[potiuk]

NOTE FOR REVIEWER: This change depends on #6596 #6702 #6696 so please review only the last commit.

For the sake of consistency, all the common components exposed directly
in 'airflow' init.py should be imported from there rather than
from sub-packages. This is important also for back-porting
purposes (AIP-21) to make sure that we are using stable imports.

This change does it and implements pre-commit that prevents importing
those common imports differently.

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-6005

Description

• Here are some details about my PR, including screenshots of any UI changes:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

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

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

[potiuk]

I think I nicely solved the problem of old imports. I figured out a nice way how to throw a deprecation warning in case someone import "from airflow impot AirflowException". It will still work, but during tests/execution it will log a deprecation warning and also you will see this deprecation warning in IDE:

This is a I think a very straightforward way to avoid potential cyclic imports like the ones described in #6596 and opens up the possibility of removing the class from 'airflow' package in the future.

I have one problem though @mik-laj maybe you can help solving it. I added a new "deprecations" package where i will put all the "deprecated" classes. I had to remove it in autoapi_ignore because otherwise there were two candidates for AirflowException class in autoapi generation. Now I have another warning - when I generate doc I have the "missing module" error:

[AutoAPI] Mapping Data
WARNING: Cannot resolve import of unknown module airflow.deprecations in airflow
[AutoAPI] Rendering Data

I tried to add airflow.deprecations to 'autodoc_mock_imports' but it did not help. Can you please help with solving this?

[potiuk]

I think I found a nice (though a but hackish :) ) way of making the docs build @mik-laj and others. See the change :).

[ashb]

Oh, I think I see the issue with both of my comments:

Because airflow/__init__.py unconditionally imports from airflow.deprecations import AirflowException it would always run.

Could we use https://docs.python.org/3/library/importlib.html#importlib.util.LazyLoader (new in v3.5) instead? That doesn't seem to work for an pip install -e environment so no. https://pypi.org/project/lazy-import/ might be an option though. Oh, this module is GPLv3 so won't work.

And as for the "two candidates for AirflowException class in autoapi generation": putting __all__ = ['AirflowException'] in the airflow/deprecations/airflow_exception_deprecated.py might help with that.

[potiuk]

@ashb - I will try those approaches. First I have to fix the #6596 which is the base for that one and then will try some experimenting.

[potiuk]

@ashb - I looked at the options and now I implemented quickly version that does not do deprecation, but pre-commits. It requires:

• 'from airflow import AirflowException' in "providers" and "contrib"
• 'from airflow.exceptions import AirflowException' elsewhere

I think that might make perfect sense rather than deprecating. It's even more "constraining" for core developers. The check is pretty much self-explanatory:

[potiuk:/code/airflow] remove-airflow-airflow-exception-imports+ 3d11h32m58s ± pre-commit run airflow-exception --all-files
Make sure AirflowException is imported from airflow.exceptions in core.......................Passed
Make sure AirflowException is imported from airflow outside of core..........................Passed
[potiuk:/code/airflow] remove-airflow-airflow-exception-imports+ 3d11h33m25s ± pre-commit run airflow-settings --all-files
Make sure settings are imported individually as local imports.......................Passed

[potiuk]

@ashb @mik-laj @feluelle -> That's the next one resulting from the untangling cyclic deps. This one should be helpful in keeping consistent import for common classes inside airflow codebase.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@da088b3). Click here to learn what that means.
The diff coverage is 87.57%.

@@            Coverage Diff            @@
##             master    #6597   +/-   ##
=========================================
  Coverage          ?   83.79%           
=========================================
  Files             ?      669           
  Lines             ?    37668           
  Branches          ?        0           
=========================================
  Hits              ?    31565           
  Misses            ?     6103           
  Partials          ?        0

Impacted Files	Coverage Δ
airflow/utils/sqlalchemy.py	91.52% <ø> (ø)
airflow/operators/mssql_to_hive.py	100% <ø> (ø)
airflow/contrib/auth/backends/ldap_auth.py	0% <0%> (ø)
...ow/contrib/auth/backends/github_enterprise_auth.py	0% <0%> (ø)
airflow/contrib/auth/backends/kerberos_auth.py	0% <0%> (ø)
airflow/utils/log/gcs_task_handler.py	0% <0%> (ø)
airflow/contrib/auth/backends/google_auth.py	0% <0%> (ø)
...rflow/config_templates/default_webserver_config.py	0% <0%> (ø)
airflow/contrib/auth/backends/password_auth.py	0% <0%> (ø)
airflow/jobs/base_job.py	88.57% <100%> (ø)
... and 179 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da088b3...3d1182a. Read the comment docs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[feluelle]

Suggested change

	entry: "from airflow\\.exception import.* AirflowConfigException"
	entry: "from airflow\\.exceptions import.* AirflowConfigException"

[potiuk]

Will come back to it some day :).

[potiuk]

Since we are going to get rid of the airflow.init.py most likely - this one can be closed.