You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/05/20 12:16:14 UTC
[GitHub] [airflow] uranusjr opened a new pull request #15969: Introduce subpackage airflow.compat
uranusjr opened a new pull request #15969:
URL: https://github.com/apache/airflow/pull/15969
Spawned from https://github.com/apache/airflow/pull/15397#discussion_r635942246
This introduces a shim subpackage `airflow.compat` for all code to import from, instead of ad-hoc try-except.
* 'airflow.compat.functools' module shims 'functools.cached_property' (using the third-party `cached_property` package) and 'functools.cache' (with `functools.lru_cache`).
* The 'yaml' module was moved from `airflow.utils.yaml` and improved so we can always use it instead of importing `yaml` directly.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845144987
I even think that (unlike apply_providers) the old imports in providers should stay until 3.0 . There is rather little value in this comparing to the lost backwards-compatibiity with already released (and used) versions of Airflow.
But this is perfectly ok to chang it for all the "core" parts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#discussion_r636179729
##########
File path: airflow/configuration.py
##########
@@ -97,8 +98,6 @@ def default_config_yaml() -> dict:
:return: Python dictionary containing configs & their info
"""
- import airflow.utils.yaml as yaml
Review comment:
Oh, look at that. I thought about that already :D
It probably didn't when I first wrote it but I changed it before PRing it. Or that is my excuse and I'm sticking with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845155785
Eek! Good spot Jarek.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845400186
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845111025
I've put `airflow.utils.yaml` back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845105559
> Would not that require releasing a new version of Airlfow and having all providers depend on that version?
Err, right, I didn't consider that. I'll revert the `airflow.compat.yaml` one.
The functools ones don't require provider releases since the old imports will still work, the new Airflow version will only add a shortcut to do the same thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845218053
I've reverted all changes to `airflow/providers`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845141052
> The functools ones don't require provider releases since the old imports will still work, the new Airflow version will only add a shortcut to do the same thing.
Not really. It also means that when we release new providers (they are always released from master) they will not work with the old versions of airflow that have no `airflow.compat` package.
The only `sane` way of doing it is making those changes first (adding compat) to airflow and then at some point of time when releasing providers switching to it with >= <airflow_version_with_compat>. Which I think we should do.
But I'd reserve it for major versions of Airflow.
As soon as we release 2.1 version of airflow we will add-back the check where all providers are built and installed with old version of airlflow (we temporarily removed it before releasing 2.1 and adding "apply_default" incompatibility in the same way), but it will be 2.1 now) - and in this case you will get a failing job. This way we will prevent from accidental usage of this compat in providers (until we decide new wave of providers is >=2.2 for example).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845052729
Would not that require releasing a new version of Airlfow and having all providers depend on that version? We are already going to release a new version of providers that are going to depend on Airlfow 2.1+, but as I see that will be another similar change and it will require another compatibility-breaking release of providers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#issuecomment-845144987
I even think that (unlike apply_providers) the old imports in providers should stay until 3.0 . There is rather little value in this comparing to the lost backwards-compatibiity with already released (and used) versions of Airflow.
But this is perfectly ok to change it for all the "core" parts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#discussion_r636167296
##########
File path: airflow/configuration.py
##########
@@ -97,8 +98,6 @@ def default_config_yaml() -> dict:
:return: Python dictionary containing configs & their info
"""
- import airflow.utils.yaml as yaml
Review comment:
But `cyaml` is not imported in `airflow.utils.yaml` at the top level, only in functions. Importing `airflow.utils.yaml` only pulls in `sys` and `typing`, the yaml module is always only available when a function is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #15969:
URL: https://github.com/apache/airflow/pull/15969
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#discussion_r636081204
##########
File path: airflow/cli/commands/kubernetes_command.py
##########
@@ -22,7 +22,7 @@
from kubernetes.client.api_client import ApiClient
from kubernetes.client.rest import ApiException
-import airflow.utils.yaml as yaml
+from airflow.compat import yaml
Review comment:
This one I'm not quite sure about -- it's not about compat, but different ways of installing the pyyaml module (with or without C component)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15969: Introduce subpackage airflow.compat
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15969:
URL: https://github.com/apache/airflow/pull/15969#discussion_r636141716
##########
File path: airflow/configuration.py
##########
@@ -97,8 +98,6 @@ def default_config_yaml() -> dict:
:return: Python dictionary containing configs & their info
"""
- import airflow.utils.yaml as yaml
Review comment:
This was intentional here and not at the module level -- importing cyaml is "relatively" slow, and I wanted to delay this unless its _needed_ as `airflow.configuration` is in the critical path right now when importing `airflow` (yes, it shouldn't be.), and this slows down all CLI commands.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org