Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use importlib_metadata with compat to Python 3.10/3.12 stdlib #38366

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

Taragolis
Copy link
Contributor

That what I've accidentally found, that we install importlib_metadata only for Python 3.8 however in some places we might use in more modern versions of the Python when we use

try:
    import importlib_metadata as metadata
except ImportError:
   from importlib import metadata  

Right now importlib_metadata as well as (importlib_resources) presented into the Docker Image (2.8.4rc1), more like it comes from one of the dependency.
So this could be a situation that we unintendedly use some features from the backported packages and it works as expected, because this package also provide new feature which will added into the stdlib in the future, see compat table

importlib_metadata stdlib
7.0 3.13
6.5 3.12
4.13 3.11
4.6 3.10
1.4 3.8

I've tried to found places where the behavior in stdlib changed (https://docs.python.org/3/library/importlib.metadata.html), and only found two places which we use in Airflow code base

  • Entrypoints: in 3.6 (not relevant), 3.10 and 3.12
  • Distribution metadata: In 3.10 (maybe also not our case)

This PR is attempt to prevent that something work just because some dependency install importlib_metadata and our code just always use importlib_metadata if it installed. This might help to detect some minor/major incompatabilities


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Mar 21, 2024
@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:plugins labels Mar 21, 2024
@Taragolis
Copy link
Contributor Author

Taragolis commented Mar 21, 2024

Not related to this one, but we also use importlib_resources but only in one place for the importlib.resources.files feature which added in Python 3.9 and changed in 3.12 (doesn't affect us)

if sys.version_info >= (3, 9):
from importlib.resources import files as resource_files
else:
from importlib_resources import files as resource_files

@Taragolis
Copy link
Contributor Author

Ok.. seems like only one test should be fixed

@potiuk potiuk added this to the Airflow 2.9.0 milestone Mar 21, 2024
@Taragolis
Copy link
Contributor Author

Ok.. seems like only one test should be fixed

Need to fix mocked method

@Taragolis Taragolis added type:misc/internal Changelog: Misc changes that should appear in change log type:improvement Changelog: Improvements and removed type:misc/internal Changelog: Misc changes that should appear in change log type:improvement Changelog: Improvements labels Mar 22, 2024
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all the cases and confirmed it looks good according to the stdlib changes.

@uranusjr @ashb as you were involved in those, I think your pairs of eyes will be useful here as well.

@potiuk
Copy link
Member

potiuk commented Mar 25, 2024

Rebase and merge @Taragolis ?

@Taragolis
Copy link
Contributor Author

Rebase and merge @Taragolis ?

Yep, I will do it in the morning, I'm out of laptop

@potiuk potiuk merged commit b496dc8 into main Mar 26, 2024
85 checks passed
@Taragolis Taragolis deleted the bump-importlib-metadata branch March 28, 2024 10:26
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:plugins full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants