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

Include backtracking in user guide #9040

Merged
merged 14 commits into from Nov 14, 2020

Conversation

ei8fdb
Copy link
Contributor

@ei8fdb ei8fdb commented Oct 23, 2020

This PR is to include backtracking information in the pip user guide to provide users with information; 1) why this occurs, and 2) how the user can possibly reduce its occurance. (GH issue #9039)

In particular I'd appreciate close review of the section "Possible ways to reduce backtracking occuring" (1-4).

@ei8fdb ei8fdb added C: dependency resolution About choosing which dependencies to install type: docs Documentation related labels Oct 23, 2020
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. But I'd prefer it to be put in its "correct" location, and the confusing "Backtracking user guide information" section get removed or rewritten accordingly.

docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
@ei8fdb
Copy link
Contributor Author

ei8fdb commented Oct 23, 2020

@pfmoore thanks for the review. What's the preferred "pip-way" PRs are updated? A subsequent patch made to this PR, or this one to be reverted and 1 single newer PR be made?

@brainwane
Copy link
Member

You don't need to close the old one and open a new one. You can update your pull request and we recommend you do that (instead of closing the old PR and opening a new one) because then it's easier to see the history of what you've done, to to check that you've responded to previous reviewers' comments, and to preserve links we've mentioned (in comments) with other issues and PRs.

@pfmoore
Copy link
Member

pfmoore commented Oct 23, 2020

Specifically, just push an extra commit to your branch and the PR will get updated automatically.

Copy link
Member

@brainwane brainwane left a comment

Choose a reason for hiding this comment

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

Thank you @ei8fdb for the work! I have some suggestions - mostly little polish things. I also marked a few places where it would be nice to add some further context for readers, but I am fine with delaying those till a future pull request if you are running short on time!

docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member

@ei8fdb I think you missed a few review comments that got collapsed by GitHub.

@pradyunsg pradyunsg added this to the 20.3 milestone Oct 26, 2020
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

The prose looks good to me!

A few minor suggestions toward polishing up.

news/9039.doc.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
Co-authored-by: Pradyun Gedam <3275593+pradyunsg@users.noreply.github.com>
Copy link
Member

@brainwane brainwane 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 all the improvements! Still a few places to fix but we're nearly done! :-)

docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

It'd be nice to reduce the usage of pronouns just a tiny bit, since I had a bit of trouble following what "it" referred to while reading.

Looks great other than a couple more nit-picks! :)

docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
docs/html/user_guide.rst Outdated Show resolved Hide resolved
Comment on lines 1361 to 1364
This backtrack behaviour can end in 2 ways - either 1) it will
successfully find a set of packages it can install (good news!), or 2) it will
eventually display `resolution impossible <https://pip.pypa.io/en/latest/user_guide/#id35>`__ error
message (not so good).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should make this a numbered list?

1. it will successfully..., or
2. it will eventually...

docs/html/user_guide.rst Outdated Show resolved Hide resolved
Co-authored-by: Pradyun Gedam <3275593+pradyunsg@users.noreply.github.com>
Co-authored-by: Sumana Harihareswara <sh@changeset.nyc>

For package maintainers during the development, give pip some help by
creating constraint files for the dependency tree. This will reduce the
number of versions it will try.
Copy link
Member

Choose a reason for hiding this comment

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

In a later, followup PR, it would be nice for someone to expand this section and add more details.

@brainwane
Copy link
Member

Looks like once you address the notes on lines 1364, 1470, and 1474 we'll be set to merge!

Copy link
Contributor Author

@ei8fdb ei8fdb left a comment

Choose a reason for hiding this comment

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

@pradyunsg All comments have been ready so this is ready to go. 🥇 😄 Thanks all for the comments.

@pradyunsg
Copy link
Member

This has failures due to the linters: You can see the errors if you click "Details" for the Linting / Ubuntu check above.

You can also run the linters locally -- once you have tox installed, running tox -e lint should run the linters on your system, and give you the output locally.

@brainwane
Copy link
Member

I just made a commit that would fix the linting issues, and fixes 2 other little things in the docs as well. @pradyunsg feel free to add it to this PR and then merge the PR.

@pradyunsg
Copy link
Member

https://github.com/pypa/pip/runs/1334180482#step:8:35

Well, even that commit fails on the linter. :(

@pradyunsg pradyunsg merged commit 5eae3c9 into pypa:master Nov 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: dependency resolution About choosing which dependencies to install type: docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants