Use importlib_metadata backport to avoid CLI UserWarning#10070
Use importlib_metadata backport to avoid CLI UserWarning#10070jrbourbeau merged 5 commits intodask:mainfrom
importlib_metadata backport to avoid CLI UserWarning#10070Conversation
415258d to
a6ea3a2
Compare
| # https://github.com/python/cpython/issues/98706 | ||
| # TODO: when python 3.12 is support is added this should be a | ||
| # conditional dependency | ||
| "importlib_metadata >= 4.13.0", |
There was a problem hiding this comment.
I originally had this as:
"importlib_metadata >= 4.13.0; python_full_version < '3.10.9' or (python_version == '3.11' and python_full_version < '3.11.1')"and then in the code:
if sys.version_info < (3, 10, 9) or ((3, 11) <= sys.version_info < (3, 11, 1)):
from importlib_metadata import entry_points
else:
from importlib.metadata import entry_pointshowever:
- the conda recipe only supports conditional dependencies on major versions eg:
- importlib_metadata # py<312 - it made the code a bit complicated on every importlib.metadata import,
- we don't support 3.12 yet so I made it a mandatory release with a
TODO:to make it conditional when we add 3.12 support.
There was a problem hiding this comment.
We could probably use Jinja in the Conda recipe to add a more subtle condition, but that might not be worth it if we already prefer importlib_metadata for other reasons
a6ea3a2 to
70d3b94
Compare
70d3b94 to
dc8329d
Compare
|
I tried this branch out on the distributed PR action and got: https://github.com/dask/distributed/actions/runs/4427020593/jobs/7764104973#step:19:96 |
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @graingert! Overall the changes here look reasonable
Is it easy to add a test for this? Otherwise, would the lack of a warning over in distributeds CI suffice?
| ] | ||
| for package in packages: | ||
| v = Version(importlib.metadata.version(package)) | ||
| v = Version(importlib_metadata.version(package)) |
There was a problem hiding this comment.
Is importlib version parsing also buggy? If so, using importlib_metadata here sounds good, but if not I'd prefer to stick with importlib.metadata from the stdlib where possible
There was a problem hiding this comment.
I think it's cleaner to always use the backport throughout the whole project rather than have different versions of importlib[._]metadata used in different places
There was a problem hiding this comment.
especially now that this PR removes entry_points from dask.compatibility I don't want to have some modules with import importlib_metadata and others with import importlib.metadata
There was a problem hiding this comment.
I disagree this is cleaner. That said, the only place this comes up is in a single test that's xfailed -- so not a big deal either way. Let's just go with what you have here
| # https://github.com/python/cpython/issues/98706 | ||
| # TODO: when python 3.12 is support is added this should be a | ||
| # conditional dependency | ||
| "importlib_metadata >= 4.13.0", |
I looked into duplicating the strategy in |
|
@graingert could you add diff --git a/continuous_integration/scripts/test_imports.sh b/continuous_integration/scripts/test_imports.sh
index 1f259cc6e..1f286ec97 100644
--- a/continuous_integration/scripts/test_imports.sh
+++ b/continuous_integration/scripts/test_imports.sh
@@ -5,7 +5,7 @@ set -o errexit
test_import () {
echo "Create environment: python=$PYTHON_VERSION $1"
# Create an empty environment
- mamba create -q -y -n test-imports -c conda-forge python=$PYTHON_VERSION packaging pyyaml fsspec toolz partd click cloudpickle $1
+ mamba create -q -y -n test-imports -c conda-forge python=$PYTHON_VERSION packaging pyyaml fsspec toolz partd click cloudpickle importlib_metadata $1
conda activate test-imports
if [[ $1 =~ "distributed" ]]; then
# dask[distributed] depends on the latest version of distributed |
@jrbourbeau done |
importlib_metadata backport to avoid CLI UserWarning
pre-commit run --all-files