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

implement uv pip tree #3859

Merged
merged 48 commits into from
Jun 21, 2024
Merged

implement uv pip tree #3859

merged 48 commits into from
Jun 21, 2024

Conversation

ChannyClaus
Copy link
Contributor

@ChannyClaus ChannyClaus commented May 27, 2024

Summary

resolves #3272

  1. added it as a new subcommand rather than a flag on an existing command since that seems more consistent with cargo tree + cleaner code organization, but can make changes if it's preferred the other way.
  2. throws an error on finding a cyclic dependency, which is more similar to cargo than pipdeptree (pipdeptree seems to ignore the packages that are part of the dependency cycle and display the rest).

Test Plan

added tests 🌵

Copy link

codspeed-hq bot commented May 27, 2024

CodSpeed Performance Report

Merging #3859 will not alter performance

Comparing ChannyClaus:uv-pip-tree (828fc7d) with main (e0ad649)

Summary

✅ 13 untouched benchmarks

@ChannyClaus ChannyClaus marked this pull request as draft May 29, 2024 03:11
@ChannyClaus
Copy link
Contributor Author

discovered a bit of an issue - the current implementation goes through all extras when constructing the graph, resulting in a cycle way more often. seems like pipdeptree just ignores extras entirely; let me know if y'all have any thoughts but probably going to look into doing the same here.

@eth3lbert
Copy link
Contributor

discovered a bit of an issue - the current implementation goes through all extras when constructing the graph, resulting in a cycle way more often. seems like pipdeptree just ignores extras entirely; let me know if y'all have any thoughts but probably going to look into doing the same here.

I've actually tried adding support for extras to pipdeptree, and it partially works. However, I haven't found the time to clean it up and submit a pull request.

My approach is to first consider all extras, but respect the marker (excluding the extra part). Then, while building the extra map, prune impossible extra branches that lack required packages or are unreachable nodes.

However, this approach isn't perfect. It determines extra branches based on the installed packages and tries to infer backwards, leading to situations where it can't determine the root package requesting the extra installation correctly. This can happen when two separate packages have similar extra packages.

Another approach for the tree might be to leverage the manifest/lock file, which is also used by multiple package management tools like Cargo, pnpm, Poetry, etc. This might be more accurate.

My other concern is the tree display for packages with extras, especially those with multiple extras. How should we handle them: aggregate or split them in the display?

@ChannyClaus
Copy link
Contributor Author

discovered a bit of an issue - the current implementation goes through all extras when constructing the graph, resulting in a cycle way more often. seems like pipdeptree just ignores extras entirely; let me know if y'all have any thoughts but probably going to look into doing the same here.

I've actually tried adding support for extras to pipdeptree, and it partially works. However, I haven't found the time to clean it up and submit a pull request.

My approach is to first consider all extras, but respect the marker (excluding the extra part). Then, while building the extra map, prune impossible extra branches that lack required packages or are unreachable nodes.

However, this approach isn't perfect. It determines extra branches based on the installed packages and tries to infer backwards, leading to situations where it can't determine the root package requesting the extra installation correctly. This can happen when two separate packages have similar extra packages.

Another approach for the tree might be to leverage the manifest/lock file, which is also used by multiple package management tools like Cargo, pnpm, Poetry, etc. This might be more accurate.

My other concern is the tree display for packages with extras, especially those with multiple extras. How should we handle them: aggregate or split them in the display?

hmm yeah, given the complexity, maybe we shall go with what pipdeptree does for the time being then?

@ChannyClaus ChannyClaus force-pushed the uv-pip-tree branch 6 times, most recently from ec6763e to 48f35d4 Compare May 31, 2024 04:07
@ChannyClaus ChannyClaus marked this pull request as ready for review May 31, 2024 04:10
@ChannyClaus ChannyClaus force-pushed the uv-pip-tree branch 2 times, most recently from 0607fdf to 0c39d00 Compare May 31, 2024 14:14
@ChannyClaus
Copy link
Contributor Author

handled the case where there's a dependency cycle (test at https://github.com/astral-sh/uv/pull/3859/files#diff-ada61e3d2726ed1b05e633096131a477c895c737c6350b8238d9e81b2745e8eaR165)

let me know if there's anything else that y'all think should be changed 🌵

@ChannyClaus
Copy link
Contributor Author

@charliermarsh let me know if this is kinda low-prio to get in now - wasn't sure if i should keep rebasing on main 😅

@zanieb
Copy link
Member

zanieb commented Jun 15, 2024

Charlie's out next week, if I have time I'll take a look!

@ChannyClaus
Copy link
Contributor Author

Charlie's out next week, if I have time I'll take a look!

thanks! seems like something strange is going on with windows test after rebasing - will try to take a look more this week...

@zanieb zanieb self-requested a review June 17, 2024 10:46
@zanieb
Copy link
Member

zanieb commented Jun 20, 2024

So I gave this a try today, just to get a sense for the user experience and the output for a small project looks like this

❯ uv pip tree
editables v0.5
packse v0.0.0
└── chevron-blue v0.2.1
└── hatchling v1.24.2
    └── packaging v24.1
    └── pathspec v0.12.1
    └── pluggy v1.5.0
    └── trove-classifiers v2024.5.22
└── msgspec v0.18.6
└── pyyaml v6.0.1
└── setuptools v70.0.0
└── twine v5.1.0
    └── pkginfo v1.11.1
    └── readme-renderer v43.0
        └── nh3 v0.2.17
        └── docutils v0.21.2
        └── pygments v2.18.0
    └── requests v2.32.3
        └── charset-normalizer v3.3.2
        └── idna v3.7
        └── urllib3 v2.2.1
        └── certifi v2024.6.2
    └── requests-toolbelt v1.0.0
        └── requests v2.32.3 (*)
    └── urllib3 v2.2.1 (*)
    └── importlib-metadata v7.1.0
        └── zipp v3.19.2
    └── keyring v25.2.1
        └── jaraco-classes v3.4.0
            └── more-itertools v10.3.0
        └── jaraco-functools v4.0.1
            └── more-itertools v10.3.0 (*)
        └── jaraco-context v5.3.0
    └── rfc3986 v2.0.0
    └── rich v13.7.1
        └── markdown-it-py v3.0.0
            └── mdurl v0.1.2
        └── pygments v2.18.0 (*)
psutil v5.9.8
pypiserver v2.1.1
└── pip v24.0
└── packaging v24.1 (*)
ruff v0.1.15
syrupy v4.6.1
└── pytest v8.2.2
    └── iniconfig v2.0.0
    └── packaging v24.1 (*)
    └── pluggy v1.5.0 (*)
watchfiles v0.21.0
└── anyio v4.4.0
    └── idna v3.7 (*)
    └── sniffio v1.3.1

A things I notice here.

  1. The tree is not drawn with properly "connected" branches. As a contrasting example, here's the packse tree for a scenario:
❯ uv run -- packse view scenarios/examples/example.json
example-961b4c22
├── environment
│   └── python3.8
├── root
│   └── requires a
│       └── satisfied by a-1.0.0
├── a
│   └── a-1.0.0
│       └── requires b>1.0.0
│           ├── satisfied by b-2.0.0
│           └── satisfied by b-3.0.0
└── b
    ├── b-1.0.0
    ├── b-2.0.0
    └── b-3.0.0
        └── requires c
            └── unsatisfied: no versions for package
  1. The lack of extras display is very confusing, e.g. most of these dependencies are from packse and it's weird that the extras are not shown as sources. I know this was discussed elsewhere, but this seems like a big deal to me. I'm not sure I fully understand why this is hard to do.

  2. There's no indication of what the (*) means

  3. The versions seem superfluous and distracting when inspecting a dependency tree — should we make display of those opt-in?

For comparison purposes, here's the pipdeptree output for the same project:

❯ uv tool run pipdeptree --python .venv/bin/python
------------------------------------------------------------------------
packse==0.0.0
├── chevron-blue [required: >=0.2.1, installed: 0.2.1]
├── hatchling [required: >=1.20.0, installed: 1.24.2]
│   ├── packaging [required: >=23.2, installed: 24.1]
│   ├── pathspec [required: >=0.10.1, installed: 0.12.1]
│   ├── pluggy [required: >=1.0.0, installed: 1.5.0]
│   └── trove-classifiers [required: Any, installed: 2024.5.22]
├── msgspec [required: >=0.18.4, installed: 0.18.6]
├── PyYAML [required: >=6.0.1, installed: 6.0.1]
├── setuptools [required: >=69.1.1, installed: 70.0.0]
└── twine [required: >=4.0.2, installed: 5.1.0]
    ├── importlib_metadata [required: >=3.6, installed: 7.1.0]
    │   └── zipp [required: >=0.5, installed: 3.19.2]
    ├── keyring [required: >=15.1, installed: 25.2.1]
    │   ├── jaraco.classes [required: Any, installed: 3.4.0]
    │   │   └── more-itertools [required: Any, installed: 10.3.0]
    │   ├── jaraco.context [required: Any, installed: 5.3.0]
    │   └── jaraco.functools [required: Any, installed: 4.0.1]
    │       └── more-itertools [required: Any, installed: 10.3.0]
    ├── pkginfo [required: >=1.8.1, installed: 1.11.1]
    ├── readme_renderer [required: >=35.0, installed: 43.0]
    │   ├── docutils [required: >=0.13.1, installed: 0.21.2]
    │   ├── nh3 [required: >=0.2.14, installed: 0.2.17]
    │   └── Pygments [required: >=2.5.1, installed: 2.18.0]
    ├── requests [required: >=2.20, installed: 2.32.3]
    │   ├── certifi [required: >=2017.4.17, installed: 2024.6.2]
    │   ├── charset-normalizer [required: >=2,<4, installed: 3.3.2]
    │   ├── idna [required: >=2.5,<4, installed: 3.7]
    │   └── urllib3 [required: >=1.21.1,<3, installed: 2.2.1]
    ├── requests-toolbelt [required: >=0.8.0,!=0.9.0, installed: 1.0.0]
    │   └── requests [required: >=2.0.1,<3.0.0, installed: 2.32.3]
    │       ├── certifi [required: >=2017.4.17, installed: 2024.6.2]
    │       ├── charset-normalizer [required: >=2,<4, installed: 3.3.2]
    │       ├── idna [required: >=2.5,<4, installed: 3.7]
    │       └── urllib3 [required: >=1.21.1,<3, installed: 2.2.1]
    ├── rfc3986 [required: >=1.4.0, installed: 2.0.0]
    ├── rich [required: >=12.0.0, installed: 13.7.1]
    │   ├── markdown-it-py [required: >=2.2.0, installed: 3.0.0]
    │   │   └── mdurl [required: ~=0.1, installed: 0.1.2]
    │   └── Pygments [required: >=2.13.0,<3.0.0, installed: 2.18.0]
    └── urllib3 [required: >=1.26.0, installed: 2.2.1]
pipdeptree==2.23.0
├── packaging [required: >=23.1, installed: 24.1]
└── pip [required: >=23.1.2, installed: 24.1]
psutil==5.9.8
pypiserver==2.1.1
├── packaging [required: >=23.2, installed: 24.1]
└── pip [required: >=7, installed: 24.1]
syrupy==4.6.1
└── pytest [required: >=7.0.0,<9.0.0, installed: 8.2.2]
    ├── iniconfig [required: Any, installed: 2.0.0]
    ├── packaging [required: Any, installed: 24.1]
    └── pluggy [required: >=1.5,<2.0, installed: 1.5.0]
watchfiles==0.22.0
└── anyio [required: >=3.0.0, installed: 4.4.0]
    ├── idna [required: >=2.8, installed: 3.7]
    └── sniffio [required: >=1.1, installed: 1.3.1]

Note they display the tree correctly as I requested. It's nice that they show version requirements as well as the installed version, but I still feel like it's way too hard to read as a default output. It seems entirely fine to add support for that separately.

@zanieb
Copy link
Member

zanieb commented Jun 20, 2024

I also find the cargo behavior around cycles pretty unhelpful as it makes it very difficult to understand how the cycle is happening. What's the problem with displaying cycles? It seems like if a depends on b and b depends on c which depends on a we'd just display something like

- a
    - b
       - c
          - a

Where if a package is already present at the top-level we omit display of its tree?

@ChannyClaus
Copy link
Contributor Author

ChannyClaus commented Jun 20, 2024

The tree is not drawn with properly "connected" branches.

ah my bad - let me try to fix this today.

There's no indication of what the (*) means

i can definitely add some sort footnote for this in the output; was just following what cargo tree does.

The versions seem superfluous and distracting when inspecting a dependency tree — should we make display of those opt-in?

i'd guess it would be more common default to display the versions but putting it behind a flag seems also fine.

The lack of extras display is very confusing, e.g. most of these dependencies are from packse and it's weird that the extras are not shown as sources. I know this was discussed elsewhere, but this seems like a big deal to me. I'm not sure I fully understand why this is hard to do.

i'll make a separate comment on this with an example, when i was playing around with the version that includes extra, installing a few popular packages would create a dependency cycle. seemed like it would be too limiting as a feature to fail on common set of installed packages.

we'd just display something like

a
|--> b
        |->> c

this is actually easier; if preferred, i can revert the code to print the above output? (and error underneath?)

@zanieb
Copy link
Member

zanieb commented Jun 20, 2024

ah my bad - let me try to fix this today.

Note this is pretty painful. The packse implementation may be a helpful reference.

re (*) was just following what cargo tree does.

What does it mean though? :)

Regarding cycles in extras: I'm not sure why they need to be an error though? It seems like we should be able to display them.

@ibraheemdev
Copy link
Member

The tree is not drawn with properly "connected" branches. As a contrasting example, here's the packse tree for a scenario:

Another thing that is missing is the current package as the root of the tree.

There's no indication of what the (*) means

(*) is taken from cargo to represent a duplicated package that has already been expanded elsewhere. Cargo has the --no-dedupe flag to fully expand these nodes. De-duplicating by default would likely be far too noisy. The other alternative is to omit the (*) entirely, but I think that would end up being more confusing.

Cycles could similarly use (*) and not be special-cased, I think.

@ibraheemdev
Copy link
Member

cargo tree has some other options, such as --prune and especially --depth, that we likely will want to add eventually.

@zanieb
Copy link
Member

zanieb commented Jun 20, 2024

Ah thank you @ibraheemdev. Yeah I'm suggesting something like using (*) for the cycles. We could do (**) as well. Notably we couldn't expand them though.

@ChannyClaus
Copy link
Contributor Author

@zanieb
Copy link
Member

zanieb commented Jun 21, 2024

The branching looks great now! A footnote seems reasonable for * I think we can skip using ** and just use * with a different footnote when a --no-dedupe flag is used.

Thoughts on extras? Should we pursue it as a follow-up?

@ChannyClaus
Copy link
Contributor Author

The branching looks great now! A footnote seems reasonable for * I think we can skip using ** and just use * with a different footnote when a --no-dedupe flag is used.

Thoughts on extras? Should we pursue it as a follow-up?

yah, follow-up sounds good. i think it's confusing both ways. if we do show extras, it would be confusing if the user didn't install it with extras. if we do not show extras, as you pointed out, the user will be surprised by them not being linked in the tree output.

i'll add a footnote shortly!

@ChannyClaus
Copy link
Contributor Author

ChannyClaus commented Jun 21, 2024

done 🌵

$ cargo run -q pip tree
uv-cyclic-dependencies-c v0.1.0
└── uv-cyclic-dependencies-a v0.1.0
    └── uv-cyclic-dependencies-b v0.1.0
        └── uv-cyclic-dependencies-a v0.1.0 (*)
Note: (*) indicates the package has been `de-duplicated`.
The dependencies for the package have already been shown elsewhere in the graph, and so are not repeated.

should i use a period not a semi-colon - i feel like i overuse it

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this (and your patience!)

Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

Great work! Some of the other things we discussed can be addressed in follow-up PRs.

@ibraheemdev ibraheemdev merged commit dd45fce into astral-sh:main Jun 21, 2024
47 checks passed
@ibraheemdev ibraheemdev mentioned this pull request Jun 21, 2024
6 tasks
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.

Add a pipdeptree like output
6 participants