Repository navigation
fix(scripts): replace pkg_resources in collect_info with importlib.metadata - #2355
Ludwig J. Marx (LudwigJMarx) wants to merge 3 commits into
Conversation
The bug report template points users at collect_info.py for their environment details, and nothing held the script to actually running. Whether pkg_resources resolves depends on the installed setuptools, so the test would otherwise pass or fail by accident. It runs the script in a subprocess behind a meta_path finder that blocks pkg_resources, which keeps it red on any setuptools version while the import is in the file. The second case carries a skipIf: where cython is installed, a missing package from that list cannot be shown. Both fail on this commit. Refs microsoft#2354
…tadata setuptools stopped shipping pkg_resources with 82.0.0. Measured rather than taken from the changelog: 80.9.0 ships it, 81.0.0 still ships it behind a deprecation warning, 82.0.0 does not. Since Python 3.12 venv no longer seeds setuptools either, so the module can be absent regardless. importlib.metadata has been in the standard library since 3.8 and requires-python is >=3.8.0, so this adds no dependency. Checked on setuptools 81, where both APIs exist, that they report the same versions across the awkward names: ruamel.yaml, python-redis-lock, pydantic-settings, setuptools-scm. No differences. The second cause sits behind the first: REQUIRED lists wheel and cython, which are in build-system.requires rather than dependencies, so a plain install does not have them and the script died part way through its output. A missing package now prints a line. Dropping those two names from the list was rejected. For a diagnostic script the useful answer is that a package is missing, not an abort. Refs microsoft#2354
Parafee41 (koriyoshi2041)
left a comment
There was a problem hiding this comment.
The stdlib replacement preserves the existing output shape for installed distributions and, importantly, turns absent build-only packages into useful diagnostics instead of aborting the report.
I checked exact head 38a30864: the focused tests pass (2/2), git diff --check is clean, and python scripts/collect_info.py all exits successfully in a Python 3.12 environment where both wheel and cython are absent and reports each as not installed.
…hine REQUIRED lists wheel and cython, which sit in build-system.requires, so a plain install does not have them. The test used that: it asserted the "not installed" line for cython and carried a skipIf for machines that do have cython. On such a machine the test is skipped and the except branch carries no test at all. Measured rather than assumed. A cython dist-info on the path makes all 26 names in REQUIRED resolve. In that state the old test reports "1 passed, 1 skipped" with a bare raise in place of the except branch, so the mutation survives. The driver now makes one name missing, the same way it already blocks pkg_resources: it wraps importlib.metadata.version and raises PackageNotFoundError for cython. The skipIf is gone, both branches run everywhere, and both mutations (bare raise, and back to pkg_resources) fail the tests with and without cython present. Refs microsoft#2354
|
While looking at the same question on #2353, I found the same shape here, and the test did not catch it either.
Measured, not assumed. A
The driver now makes one name missing, the same way it already blocks The point raised in the review on #2353 applies to this PR as well. Rest of the suite on |
|
Rechecked the updated head |
Description
scripts/collect_info.pyusedpkg_resources, which setuptools stoppedshipping with 82.0.0, and it treated every name in
REQUIREDas installed.Switched to
importlib.metadataand made a missing package print a lineinstead of raising.
Motivation and Context
Refs #2354.
The bug report template points users at this script for their environment
details, so it fails exactly when someone is trying to file a report.
importlib.metadatahas been in the standard library since Python 3.8 andrequires-pythonis>=3.8.0, so this adds no dependency.Two causes, and the second is only reachable once the first is fixed:
import pkg_resourcesstops the script before it starts, andREQUIREDlistswheelandcython, which sit inbuild-system.requiresrather than independencies. Both stand in the way of the same outcome, so they are handledtogether rather than in two PRs.
Rejected: dropping
wheelandcythonfromREQUIRED. For a diagnosticscript the useful answer is that a package is missing, not an abort, and the
same handling covers later edits to that list.
How Has This Been Tested?
pytest qlib/tests/test_all_pipeline.pyunder upper directory ofqlib.New file
tests/test_collect_info.py. The result must not depend on whicheversetuptools the machine happens to hold, so the test runs the script in a
subprocess behind a
meta_pathfinder that blockspkg_resources. That keepsit red on any setuptools version while the import is in the file. The two
commits carry the evidence: red on
8c7e8698, green on38a30864.Before the fix:
After the fix:
And by hand, the way the template asks for it
(
cd scripts && python collect_info.py all), exit 0:Counter-check that the replacement reports the same values: in a venv on
setuptools 81, where both APIs still exist,
pkg_resources.get_distribution(x).versionagainstimportlib.metadata.version(x)over seven packages includingruamel.yaml,python-redis-lock,pydantic-settingsandsetuptools-scm: zerodifferences.
Rest of the suite, same command as CI (
cd tests && pytest . -m "not slow"):The two extra are the new tests.
black -l 120 --checkclean,pylint10.00/10 onscripts/collect_info.py.Environment: Python 3.12.13, setuptools 84.0.0, MacOS 26.6.2 arm64, commit
be72549.
Types of changes