You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ashb (via GitHub)" <gi...@apache.org> on 2023/02/23 13:50:39 UTC
[GitHub] [airflow] ashb opened a new pull request, #29723: Don't use importlib.metadata to get Version for speed
ashb opened a new pull request, #29723:
URL: https://github.com/apache/airflow/pull/29723
As discovered by @uranusjr in other PRs, loading the Metadata info at runtime is surprisingly expensive.
Recent versions of setuptools (including the one we already say we depend upon in pyproject.toml) have the ability to pull the version from an attribute using an "ast-eval" method so this keeps the property of there being a single source of truth for the Airflow version, it just moves that places to airflow/__init__.py.
You might wonder why this particular case matters at runtime? In the grand scheme of things it likely doesn't, except that the airflow/operators/python.py imports this at the top level (or did before this PR) which made me look in to it and discover a quick win here.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29723:
URL: https://github.com/apache/airflow/pull/29723#discussion_r1115731307
##########
setup.py:
##########
@@ -907,9 +904,7 @@ def include_provider_namespace_packages_when_installing_from_sources() -> None:
write_version()
setup(
distclass=AirflowDistribution,
- version=version,
extras_require=EXTRAS_DEPENDENCIES,
- download_url=("https://archive.apache.org/dist/airflow/" + version),
Review Comment:
Likewise here, it doesn't seem that unreasonable to just link to all the downloads here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29723:
URL: https://github.com/apache/airflow/pull/29723#discussion_r1115730839
##########
setup.py:
##########
@@ -193,7 +190,7 @@ def git_version(version_: str) -> str:
if repo.is_dirty():
return f".dev0+{sha}.dirty"
# commit is clean
- return f".release:{version_}+{sha}"
+ return f".release:{sha}"
Review Comment:
This changes format of the git tag, but given this is shown directly beneath the actual version in the UI it seemed easier to change it this way, rather than have to have this do the ast eval to get version too.
If anyone thinks otherwise I can change the style 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29723:
URL: https://github.com/apache/airflow/pull/29723#issuecomment-1442254077
There are some more references to `version` that need to be changed
```
scripts/ci/pre_commit/pre_commit_update_versions.py:31: error: Module "setup"
has no attribute "version"; maybe "git_version"? [attr-defined]
from setup import version # isort:skip
^
scripts/in_container/run_migration_reference.py:34: error: Module "setup" has
no attribute "version"; maybe "git_version"? [attr-defined]
from setup import version as _airflow_version
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29723:
URL: https://github.com/apache/airflow/pull/29723#discussion_r1115729277
##########
airflow/operators/python.py:
##########
@@ -690,9 +689,11 @@ def _is_pendulum_installed_in_target_env(self) -> bool:
return False
def _get_airflow_version_from_target_env(self) -> str | None:
+ from airflow import __version__ as airflow_version
+
try:
result = subprocess.check_output(
- [self.python, "-c", "from airflow import version; print(version.version)"], text=True
+ [self.python, "-c", "from airflow import __version__; print(__version__)"], text=True
Review Comment:
This worked in 2.0.0
```
$ docker run --rm -ti --entrypoint python apache/airflow:2.0.0 -c "from airflow import __version__; print(__version__)"
2.0.0
```
I didn't test any older, but I think this is fine.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #29723:
URL: https://github.com/apache/airflow/pull/29723#discussion_r1115998413
##########
airflow/version.py:
##########
@@ -17,20 +17,5 @@
# under the License.
from __future__ import annotations
-__all__ = ["version"]
-
-try:
- import importlib_metadata as metadata
-except ImportError:
- from importlib import metadata # type: ignore[no-redef]
-
-try:
- version = metadata.version("apache-airflow")
-except metadata.PackageNotFoundError:
- import logging
-
- log = logging.getLogger(__name__)
- log.warning("Package metadata could not be found. Overriding it with version found in setup.py")
- from setup import version
-
-del metadata
+# Compat -- somethings access `airflow.version.version` directly
+from airflow import __version__ as version # noqa: F401
Review Comment:
Don’t need to `noqa` if you keep the `__all__` line
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29723:
URL: https://github.com/apache/airflow/pull/29723#discussion_r1116041132
##########
airflow/version.py:
##########
@@ -17,20 +17,5 @@
# under the License.
from __future__ import annotations
-__all__ = ["version"]
-
-try:
- import importlib_metadata as metadata
-except ImportError:
- from importlib import metadata # type: ignore[no-redef]
-
-try:
- version = metadata.version("apache-airflow")
-except metadata.PackageNotFoundError:
- import logging
-
- log = logging.getLogger(__name__)
- log.warning("Package metadata could not be found. Overriding it with version found in setup.py")
- from setup import version
-
-del metadata
+# Compat -- somethings access `airflow.version.version` directly
+from airflow import __version__ as version # noqa: F401
Review Comment:
df9faa9ca0
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #29723: Don't use importlib.metadata to get Version for speed
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb merged PR #29723:
URL: https://github.com/apache/airflow/pull/29723
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org