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

replace pkg_resources with importlib.metadata and packaging #333

Merged
merged 38 commits into from
Apr 2, 2024

Conversation

xiacunshun
Copy link
Collaborator

No description provided.

@xiacunshun
Copy link
Collaborator Author

fix #175

@gaborbernat gaborbernat marked this pull request as draft March 15, 2024 17:39
@xiacunshun
Copy link
Collaborator Author

xiacunshun commented Mar 17, 2024

Distribution.requires now returns strings like ['markdown; extra == "filters"', 'markdown; extra == "markdown"'].
I print them in the tree as the following.
CT3==3.3.1
├── markdown [required: Any, extra: extra == "filters", installed: 3.5.1]
└── markdown [required: Any, extra: extra == "markdown", installed: 3.5.1]

Requirements of different extras may cause same pkgs listed in one node. But I think it is resonable.

@xiacunshun
Copy link
Collaborator Author

The result looks corret with my own tests.
I haven't configured out why the CI tests failed. Could you please take a look? @gaborbernat

@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch from 714c368 to b0578d4 Compare March 17, 2024 16:45
@kemzeb
Copy link
Collaborator

kemzeb commented Mar 18, 2024

Thanks for contributing @xiacunshun! I do see some areas in your PR that aren't related to migrating the codebase to use importlib.metadata and I think it would be best to handle these other changes in some other PR(s) and have this one focused on just the issue it hand.

Copy link
Collaborator

@kemzeb kemzeb left a comment

Choose a reason for hiding this comment

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

packaging is not apart of the Python standard library, so I guess we need to add it as a dependency in pyproject.toml.

@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch 2 times, most recently from 733c7cd to b01ea90 Compare March 18, 2024 14:12
pyproject.toml Outdated Show resolved Hide resolved
@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch 3 times, most recently from ad4efaf to aa8dc8b Compare March 19, 2024 12:42
@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch 2 times, most recently from 8bb4cee to d7b639f Compare March 20, 2024 13:02
@pawamoy pawamoy mentioned this pull request Mar 20, 2024
@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch from d7b639f to 9dff1af Compare March 21, 2024 01:40
@xiacunshun
Copy link
Collaborator Author

xiacunshun commented Apr 1, 2024

Added another change since we also were considering packages that have not been installed because their environment markers evaluate to false in our environment (e.g. "importlib-metadata; python_version < '3.8'" when I have Python v3.9):

$ pipdeptree
Warning!!! Possibly conflicting dependencies found:
* GitPython==3.1.41
 - typing-extensions [required: >=3.7.4.3, installed: ?]
* pipdeptree==2.16.3.dev2+gedf33d9.d20240401
 - pip [required: >=23.1.2, installed: 23.0.1]
* tox==4.14.2
 - importlib-metadata [required: >=7.0.1, installed: ?]
 - typing-extensions [required: >=4.9, installed: ?]
* virtualenv==20.25.1
 - importlib-metadata [required: >=6.6, installed: ?]
------------------------------------------------------------------------
covdefaults==2.3.0
└── coverage [required: >=6.0.2, installed: 7.4.4]
. . .

Recent changes calms it down:

$ pipdeptree
Warning!!! Possibly conflicting dependencies found:
* pipdeptree==2.16.3.dev2+gedf33d9.d20240401
 - pip [required: >=23.1.2, installed: 23.0.1]
------------------------------------------------------------------------
covdefaults==2.3.0
└── coverage [required: >=6.0.2, installed: 7.4.4]
. . .

Seeing the old pip-powered output, it looks like this mimics what it does when it finds packages that can't be installed in the current environment.

Does it mean that the needed requirements cannot be installed if the warning appears? Is that a problem should alarm?

Edit: If so, I think we should warn them out in another way maybe?

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 1, 2024

No, we shouldn't warn them since packages with environment markers essentially makes it conditional whether or not they should be installed.

If there is an environment marker that expects the package to be installed on a Windows machine, pip or other package managers won't install this package on a MacOS machine. So there shouldn't be a warning (just like how it was handled before when dealing with the pip API).

@xiacunshun
Copy link
Collaborator Author

No, we shouldn't warn them since packages with environment markers essentially makes it conditional whether or not they should be installed.

If there is an environment marker that expects the package to be installed on a Windows machine, package managers won't install this package on a MacOS machine. So there shouldn't be a warning (just like how it was handled before when dealing with the pip API).

Got it, it makes sense.

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 1, 2024

I think everything looks good. The only problem I found just now has to do with using --python option:

$ pipdeptree --python venv/bin/python
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/tmp/tmpy7de5xnt/pipdeptree/__main__.py", line 10, in <module>
    from pipdeptree._models import PackageDAG
  File "/tmp/tmpy7de5xnt/pipdeptree/_models/__init__.py", line 3, in <module>
    from .dag import PackageDAG, ReversedPackageDAG
  File "/tmp/tmpy7de5xnt/pipdeptree/_models/dag.py", line 12, in <module>
    from .package import DistPackage, ReqPackage, pep503_normalize
  File "/tmp/tmpy7de5xnt/pipdeptree/_models/package.py", line 10, in <module>
    from packaging.requirements import Requirement
ModuleNotFoundError: No module named 'packaging'

Since we now depend on packaging, it looks like it needs to be included in the virtual environment. An easy way to solve this is to head over to _non_host.py and make sure packaging exists (i.e. /path/to/venv python -m pip install packaging or something similar). Haven't found a better approach to this yet.

If we're using the easy approach, we should check to see if we can force pip to only use the cached version of packaging (it should be there because the user would have installed pipdeptree with it as a dependency).

Edit: looks like pip has "on-by-default" caching, so we don't need to think about it

@xiacunshun
Copy link
Collaborator Author

xiacunshun commented Apr 2, 2024

I think this error may not need to be solved, because if you need to use non-host python which uses the dependencies in that env, so the user should ensure that the necessary dependencies exist in that environment?

edit: pip do have on-by-default cache, but it looks like it cannot be used in venv(they are separate).

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 2, 2024

I think this error may not need to be solved, because if you need to use non-host python which uses the dependencies in that env, so the user should ensure that the necessary dependencies exist in that environment?

edit: pip do have on-by-default cache, but it looks like it cannot be used in venv(they are separate).

I do see your point, but this changes how --python is ran, and when users were to upgrade to a newer version they would (maybe) need to perform one more step before they can actually run pipdeptree which I think is not ideal.

Right now I think I see two approaches:

  • Fire up a process to install packaging into the environment just before we run it i.e. /path/to/python -m pip install packaging
  • Modify PYTHONPATH (see here) to somehow include our globally-installed packaging

Regarding how pip handles caching, it seems that pip uses a single cache no matter if we're using the system environment or a virtual environment, for example:

$ python -m venv venv
$ source venv/bin/activate
(venv) $ pip cache dir
/home/vscode/.cache/pip # <--- pip's cache is stored in the user home directory, even in a virtual env
(venv) $ pip install packaging
Collecting packaging
  Using cached packaging-24.0-py3-none-any.whl (53 kB)  # <--- We snatch packaging from the cache
Installing collected packages: packaging
Successfully installed packaging-24.0

@xiacunshun
Copy link
Collaborator Author

Right now I think I see two approaches:
Fire up a process to install packaging into the environment just before we run it i.e. /path/to/python -m pip install packaging
Modify PYTHONPATH (see here) to somehow include our globally-installed packaging

The second one is better I think, I will try this.

@xiacunshun
Copy link
Collaborator Author

Regarding how pip handles caching, it seems that pip uses a single cache no matter if we're using the system environment or a virtual environment, for example:

You are right. I didn't notice it before.
Another situation is that if the distribution is installed by rpm instead of pip, pip cache cannot be used.

Found existing installation: packaging 23.1
ERROR: Cannot uninstall packaging 23.1, RECORD file not found. Hint: The package was installed by rpm.
[root@linux tools]# source /root/myenv/bin/activate
(myenv) [root@linux tools]# 
(myenv) [root@linux tools]# 
(myenv) [root@linux tools]# pip uninstall packaging
WARNING: Skipping packaging as it is not installed.
(myenv) [root@linux tools]# pip install packaging
Collecting packaging
  Downloading packaging-24.0-py3-none-any.whl.metadata (3.2 kB) <===== need to download through network
Downloading packaging-24.0-py3-none-any.whl (53 kB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 53.5/53.5 kB 1.4 MB/s eta 0:00:00
Installing collected packages: packaging
Successfully installed packaging-24.0

We should better not to download something in our progress, even in non host cases.

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 2, 2024

Another situation is that if the distribution is installed by rpm instead of pip, pip cache cannot be used.

I agree, and I think the latter approach is better since we don't have to perform any networking (as packaging should exist in whatever environment the user is in already if pipdeptree is installed) and we don't have to have users use pip to install it (as there maybe some users that use an alternative to pip/PyPI to install their packages).

src/pipdeptree/_non_host.py Outdated Show resolved Hide resolved
tests/test_non_host.py Outdated Show resolved Hide resolved
@xiacunshun xiacunshun force-pushed the replace_deprecate_pkg_resources branch from 60530f5 to 63ffb33 Compare April 2, 2024 07:17
@kemzeb
Copy link
Collaborator

kemzeb commented Apr 2, 2024

Alright, after looking at this patch a couple more times I'm not seeing any other areas of concern so I think this is good to go. Thanks @xiacunshun for the contribution!

@kemzeb kemzeb enabled auto-merge (squash) April 2, 2024 20:40
@kemzeb
Copy link
Collaborator

kemzeb commented Apr 2, 2024

Hey @gaborbernat, it looks like since I was the latest one to push to this branch I'm gonna need your approval with this one. I understand you're busy, so please review this at your convenience. Thank you!

@xiacunshun Once this PR is merged, I'll go ahead and add you as a collaborator 👍

@gaborbernat gaborbernat merged commit 91d21e3 into tox-dev:main Apr 2, 2024
10 checks passed
@kemzeb
Copy link
Collaborator

kemzeb commented Apr 2, 2024

Thanks!

Alright @xiacunshun, after you accept the invitation you will be able to manage issues and pull requests.

And since this is refactoring work, I believe we can cut a minor release (i.e. 2.17.0)

@xiacunshun xiacunshun deleted the replace_deprecate_pkg_resources branch April 3, 2024 00:05
@xiacunshun
Copy link
Collaborator Author

Thanks!

Alright @xiacunshun, after you accept the invitation you will be able to manage issues and pull requests.

And since this is refactoring work, I believe we can cut a minor release (i.e. 2.17.0)

Thanks for your invitation! I think 2.17.0 is OK. And I noticed there is an unresolved PR, if it needs to be merged, let's wait for this, and then draft the release?

@gaborbernat
Copy link
Member

Thanks!

Alright @xiacunshun, after you accept the invitation you will be able to manage issues and pull requests.

And since this is refactoring work, I believe we can cut a minor release (i.e. 2.17.0)

Thanks for your invitation! I think 2.17.0 is OK. And I noticed there is an unresolved PR, if it needs to be merged, let's wait for this, and then draft the release?

We should not wait for that. The pull request in question might be in progress for another month from all we know.

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 3, 2024

We should not wait for that. The pull request in question might be in progress for another month from all we know.

I agree. Also, even though we have removed alot of its API usage, we still need pip to generate frozen requirements. This PR removed older frozen requirement API since we assumed that we would be working with pip>=23.1.2. If we need to handle older pip versions again, we need to revert some of these changes and maybe change the existing adapter class or add new adapter classes to handle older versions of FrozenRequirement.from_dist(...).

I'll see if I can cut a minor release tomorrow!

@kemzeb
Copy link
Collaborator

kemzeb commented Apr 3, 2024

Hmm, the thought of duplicate packages when using importlib.metadata came to my mind and it looks like the discovery algorithm will need more work. I installed pytest both in my system environment and in a temporary directory. I assigned this temporary directory to PYTHONPATH and was able to see it twice, including its dependencies. Here's an example:

$ mkdir temp-site-pkgs
$ pip install --target temp-site-pkgs/ pytest
Collecting pytest
  Using cached pytest-8.1.1-py3-none-any.whl.metadata (7.6 kB)
. . .
Installing collected packages: tomli, pluggy, packaging, iniconfig, exceptiongroup, pytest
Successfully installed exceptiongroup-1.2.0 iniconfig-2.0.0 packaging-24.0 pluggy-1.4.0 pytest-8.1.1 tomli-2.0.1
$ PYTHONPATH=/workspaces/pipdeptree/temp-site-pkgs pipdeptree -a -d 0
attrs==23.2.0
Automat==22.10.0
cachetools==5.3.3
certifi==2024.2.2
cffi==1.16.0
cfgv==3.4.0
chardet==5.2.0
charset-normalizer==3.3.2
colorama==0.4.6
constantly==23.10.4
covdefaults==2.3.0
coverage==7.4.4
cryptography==42.0.5
cssselect==1.2.0
diff_cover==8.0.3
distlib==0.3.8
exceptiongroup==1.2.0                             # <--- Here
exceptiongroup==1.2.0
filelock==3.13.1
gitdb==4.0.11
GitPython==3.1.41
hyperlink==21.0.0
identify==2.5.35
idna==3.6
incremental==22.10.0
iniconfig==2.0.0                             # <--- Here
iniconfig==2.0.0
itemadapter==0.8.0
itemloaders==1.1.0
Jinja2==3.1.3
jmespath==1.0.1
lxml==5.2.1
MarkupSafe==2.1.5
nodeenv==1.8.0
packaging==24.0                         # <--- Here
packaging==24.0
parsel==1.9.0
pip==24.0
pip==23.0.1
pipdeptree==2.16.3.dev3+g91d21e3
platformdirs==4.2.0
pluggy==1.4.0                                # <--- Here
pluggy==1.4.0
pre-commit==3.7.0
Protego==0.3.0
pyasn1==0.6.0
pyasn1_modules==0.4.0
pycparser==2.22
PyDispatcher==2.0.7
Pygments==2.17.2
pyOpenSSL==24.1.0
pyproject-api==1.6.1
pytest==8.1.1                             # <--- Here
pytest==8.1.1
pytest-cov==4.1.0
pytest-mock==3.14.0
PyYAML==6.0.1
queuelib==1.6.2
requests==2.31.0
requests-file==2.0.0
Scrapy==2.11.1
service-identity==24.1.0
setuptools==69.0.3
six==1.16.0
smmap==5.0.1
tldextract==5.1.2
tomli==2.0.1                             # <--- Here
tomli==2.0.1
tox==4.14.2
Twisted==24.3.0
typing_extensions==4.10.0
urllib3==2.2.1
virtualenv==20.25.1
w3lib==2.1.2
wheel==0.42.0
zope.interface==6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants