conda-incubator / conda-tree

conda dependency tree helper
MIT License
160 stars 12 forks source link

Adding a tree view – 0.0.5 proposal #7

Closed lemeb closed 5 years ago

lemeb commented 5 years ago

Hi @rvalieris, thank you so much for this project! I wrote a few adjustments to conda-tree, tell me what you think:

Tree view

I kind of wanted to merge the best of pipdeptree and conda-tree, and so I took the liberty of forking your repo and add the tree view functionality.

It works by adding a -t/--tree option:

$ python conda-tree.py depends -t sqlite 
sqlite==3.29.0
  - ncurses [required: >=6.1,<6.2.0a0, installed: 6.1]
    - libcxx [required: >=4.0.1, installed: 8.0.1]
      - libcxxabi [required: 8.0.1, 0, installed: 8.0.1]
  - readline [required: >=8.0,<9.0a0, installed: 8.0]
    - ncurses [required: >=6.1,<6.2.0a0, installed: 6.1]
      ... (already above)

A few notes:

  1. It also works with whoneeds.
  2. You perhaps noticed that displaying the dependencies ncurses would be redundant. Hence, these dependencies are displayed once, and then hidden by default for the rest of the tree. There is a --full option to halt this behavior.
  3. -t and -r are mutually exclusive, and -r's behavior is unchanged.
  4. To avoid cycles, I removed the python package from the graph. (Of course, not every conda package relies on python, but that was the only way I found to not break everything.)

deptree subcommand

There is now a deptree subcommand, that displays the full dependency tree. This is where hiding redundant dependencies becomes really practical. (It essentially repeats conda-tree -t depends for every package in conda-tree leaves).

There is a --no-conda option to avoid conda from the dependency tree; then you can truly have an idea of what came on top of the default install.

Refactoring

I refactored a few things in the code to avoid repetitions. The argparse parser is a little more complex, but more hierarchical. For instance, it knows that -r only applies to functions relative to packages, and so on.

rvalieris commented 5 years ago

Hello, this is a great idea ! thanks for the PR. I don't have much time to look at all the changes right now but for now I will point out a two things I noticed.

lemeb commented 5 years ago

Update:

Bugfix

I think the bugs are gone, and I fixed a couple of others.

Tree view

Just for fun, I figured it might be interesting to try to display the tree the way that npm list does. It is a little harder than I thought, but it works pretty well. I also dimmed the installed version and the requirements, to make the tree easier to read. The warning that dependencies are hidden got the same treatment.

Screen Shot 2019-08-29 at 18 30 17

To handle this new refactoring, I added a state dict to store important setting values. It is passed around to the various recursive iterations of the print_dep_tree functions, and avoids setting global variables.

Warnings

Tree views (whether via the -t option or the deptree subcommand) hide redundant dependencies by default, which I think is better from a design standpoint. When one redundancy is hidden, however, I made sure that the script was printing a warning about that choice:

$ python conda-tree.py whoneeds -t jupyter_client
jupyter_client==5.3.1
   ├─ ipykernel 5.1.2 [required: any]
   │  └─ notebook 6.0.1 [required: any]
   │     ├─ jupyterlab_server 1.0.6 [required: >=4.2.0]
   │     │  └─ jupyterlab 1.0.9 [required: >=1.0.0,<2.0.0]
   │     └─ jupyterlab 1.0.9 [required: >=4.3.1]
   └─ notebook 6.0.1 [required: >=5.3.1]
      └─ dependent packages of notebook displayed above

For the sake of clarity, some redundancies have been hidden.
Please use the '--full' option to display them anyway.

Breaking changes

I forgot to mention, but my refactoring of the argument parser means that some combination of subcommands and arguments will stop working:

$ python conda-tree.py -r depends conda
usage: conda-tree.py [-h] [-p PREFIX] [-n NAME] [-v]
                     {leaves,cycles,whoneeds,depends,deptree} ...
conda-tree.py: error: unrecognized arguments: -r

Instead, one should use python conda-tree.py depends -r conda or python conda-tree.py depends conda -r, which do not currently work. I have a strong preference for this arrangement, where -r is tied to the subcommand, as it resembles the way that most CLI tools parse arguments. But you're the maintainer, so you do what you wish :)

rvalieris commented 5 years ago

great work so far ! this is looking good !

I don't think we need to skip python on make_cache_graph since its already being removed from the graph on print_dep_tree. actually, ideally, if a depends tree of python is requested we want to show the tree and then omit any other appearance of python like its already being done for other packages, to avoid the cycle, does that make sense ? the only difference is that we would still not show everything on --full.

example:

python==3.7.3
 [.....]
  └─ pip 19.2.3 [required: any]
     ├─ python 3.7.3 [required: >=3.7,<3.8.0a0]
     │  └─ dependencies of python displayed above
     ├─ setuptools 41.2.0 [required: any]
     │  ├─ certifi 2019.6.16 [required: >=2016.09]
     │  │  └─ python 3.7.3 [required: >=3.7,<3.8.0a0]
     │  │    └─ dependencies of python displayed above
     │  └─ python 3.7.3 [required: >=3.7,<3.8.0a0]
     │    └─ dependencies of python displayed above
     └─ wheel 0.33.6 [required: any]
        ├─ python 3.7.3 [required: >=3.7,<3.8.0a0]
        │ └─ dependencies of python displayed above
        └─ setuptools 41.2.0 [required: any]
          └─ dependencies of setuptools displayed above

as for the breaking changes, yeah thats alright. I didn't liked the fact the option was before the subcommand to be honest, it ended up that way because I wanted to avoid added the option twice for each subcommand, but you found a better solution already :)

lemeb commented 5 years ago

Update:

'python' dependencies

You're right on this, especially since, in conda, not every module requires python. So I put python back in the tree, and essentially defined the behavior according to three modes:

NB: --small and --full are mutually exclusive in the arg parser.

Looking ahead

There is a few things that would be easy-ish to do with the code that we have now:

rvalieris commented 5 years ago

nice !

exclude and depth options sounds very useful to have ! and I don't see harm in ordering the results as the order of the dependencies shouldn't matter.

the output for the python deps still not quite what I imagined tho, because it still repeats python once before omitting it. same thing happens with depends -t setuptools, the first appearance of setuptools is still showing. I think we could fix this by adding if len(e) > 0: state['tree_exists'].add(pkg) on line 85. if possible, I want to make this more generic to avoid these cycles regardless of the package.

┬ character: looks cool ! I think I do like this one a little better, but either one looks great already.

dependencies of python displayed above will then be a little misleading, because then the dependencies of python are being displayed above and below, but I'm not sure how to avoid that easily.

I guess its a matter of changing the wording, something more generic like "dependencies of omitted" and then a have that warning in the end explain it could work.

-r improvements: yes that sounds useful, but since this output is already a unique list, we could keep the default behavior as it is and just have --small omit python, conda. not sure what --full would do.

lemeb commented 5 years ago

i’m in a hurry, so i just corrected the line 85. tell me if that’s what you expected!

rvalieris commented 5 years ago

yeah, to be clear, on the depends -t python case the dependencies of python are showed inside the python->pip->python tree, with this extra line I suggested it won't show.

not that I think this is the best way to deal with this issue, I am still wrapping my head around the code, but just to illustrate the difference.

there's no hush though, take your time.

PertuyF commented 5 years ago

Hi guys, Very interesting work @lemeb! When do you plan to merge these changes?

rvalieris commented 5 years ago

yes ! sorry, I forgot about this.. I will merge this now. thanks such much @lemeb !!