Include already-installed matching requirements in the to_install set, so that pip-sync reliably installs dependencies

[AndydeCleyre]

Have diff include already-installed matching requirements in its to_install set.

This means that during sync, dependencies of desired packages will be (re)installed after potentially being uninstalled, regardless of environment state at time of sync, for a more consistent, predictable, and practical resulting state. Fixes #896.

If this is not desired, --no-deps can be provided via the new --pip-args option, whereby it will be passed through to pip install, and un-locked deps will be either removed or not installed, to match the lockfile exactly.

The following existing tests check the to_install set, and are herein adjusted to expect the inclusion of explicit requirements (all in test_sync.py):

• test_diff_should_do_nothing
• test_diff_should_uninstall
• test_diff_should_uninstall_with_markers
• test_diff_leave_packaging_packages_alone
• test_diff_leave_piptools_alone
• test_diff_with_matching_url_versions

Note: test_diff_with_matching_url_versions, before this change, expects pip-tools to prevent reinstallation of versioned URL requirements, but this is unnecessary as pip itself will do that, reporting "Requirement already satisfied . . .", and the test's comment is updated to reflect that. If this is incorrect under some circumstance, please correct me!

Changelog-friendly one-liner: pip-sync now ensures installation of dependencies of a locked requirement, even if those deps are not locked and the locked requirement is already installed; this can be disabled with pip-sync --pip-args=--no-deps.

Contributor checklist

• [X] Provided the tests for the changes. (Well, modified existing tests)
• [X] Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
• [ ] Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

If this were to be accepted, would the appropriate merge point be the next major version, as the behavior may not be what is currently expected? I view it as a bug fix, and do not know when or why the current behavior would be desired instead, but I am only me.

@atugushev Please have a look for discussion and review. Thanks!

[codecov[bot]]

Codecov Report

Merging #907 (16a4115) into master (26ee416) will decrease coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 16a4115 differs from pull request most recent head 28dabb8. Consider uploading reports for the commit 28dabb8 to get more accurate results

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
- Coverage   99.67%   99.67%   -0.01%     
==========================================
  Files          33       33              
  Lines        3038     3034       -4     
  Branches      327      326       -1     
==========================================
- Hits         3028     3024       -4     
  Misses          5        5              
  Partials        5        5              

Impacted Files	Coverage Δ
piptools/sync.py	100.00% <100.00%> (ø)
tests/test_sync.py	100.00% <100.00%> (ø)
piptools/repositories/pypi.py	97.10% <0.00%> (-0.02%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 26ee416...28dabb8. Read the comment docs.

[AndydeCleyre]

@atugushev sure: #987

But I will note again, this more complete pull request (#907) is required to achieve consistent and predictable behavior.

Current behavior is to sometimes act like --no-deps is provided, sometimes not. So the new ~request #987~ --pip-args provides a way to consistently achieve --no-deps behavior, but without specifying that option the user will still sometimes get that behavior and sometimes not.

[AndydeCleyre]

Before:

$ echo "pytest==5.1.2" > requirements.txt
$ pip-sync /dev/null
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync requirements.txt
$ pip freeze | wc -l
4
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4
$ pip-sync /dev/null
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4

After:

$ echo "pytest==5.1.2" > requirements.txt
$ pip-sync /dev/null
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4
$ pip-sync /dev/null
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4

[ssbarnea]

@AndydeCleyre @atugushev are you still interested in getting this ready for merging? It is a very old PR.

At least the PR title and descriptions needs a little bit of simplification.

[AndydeCleyre]

@ssbarnea

FWIW I still want this, but there is not agreement that this is the right thing to do. But AFAIK there are no other PRs addressing #896.

[ssbarnea]

@atugushev I have the impression that @webknjaz took care of this one, does it look ok now? If so, lets merge it.

[atugushev]

@ssbarnea

As @AndydeCleyre said there's still no consensus on that. My point is described here. pip-sync reflects what is written in requirements.txt. If you need to install everything in requirements.txt including deps, which for some reason is absent in the file (pip-compile failed?) then pip install -r requirements.txt should be used.

BTW with the current fix, pip-sync still acts strange with "broken" requirements.txt - uninstalls and installs sub-deps on each run, see collapsed details below.

Details

$ echo django==4.1.2 > requirements.txt

$ pip-sync
Collecting django==4.1.2
  Using cached Django-4.1.2-py3-none-any.whl (8.1 MB)
Collecting backports.zoneinfo
  Using cached backports.zoneinfo-0.2.1-cp38-cp38-macosx_10_14_x86_64.whl (35 kB)
Collecting asgiref<4,>=3.5.2
  Using cached asgiref-3.5.2-py3-none-any.whl (22 kB)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.3-py3-none-any.whl (42 kB)
Installing collected packages: sqlparse, backports.zoneinfo, asgiref, django
Successfully installed asgiref-3.5.2 backports.zoneinfo-0.2.1 django-4.1.2 sqlparse-0.4.3

$ pip-sync
Found existing installation: asgiref 3.5.2
Uninstalling asgiref-3.5.2:
  Successfully uninstalled asgiref-3.5.2
Found existing installation: backports.zoneinfo 0.2.1
Uninstalling backports.zoneinfo-0.2.1:
  Successfully uninstalled backports.zoneinfo-0.2.1
Found existing installation: sqlparse 0.4.3
Uninstalling sqlparse-0.4.3:
  Successfully uninstalled sqlparse-0.4.3
Requirement already satisfied: django==4.1.2 in ./.venv/lib/python3.8/site-packages (from -r /var/folders/l0/lnq1ghps5yqc4vkgszlcz92m0000gp/T/tmprpacgszi (line 1)) (4.1.2)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.3-py3-none-any.whl (42 kB)
Collecting asgiref<4,>=3.5.2
  Using cached asgiref-3.5.2-py3-none-any.whl (22 kB)
Collecting backports.zoneinfo
  Using cached backports.zoneinfo-0.2.1-cp38-cp38-macosx_10_14_x86_64.whl (35 kB)
Installing collected packages: sqlparse, backports.zoneinfo, asgiref
Successfully installed asgiref-3.5.2 backports.zoneinfo-0.2.1 sqlparse-0.4.3

pip-compile and pip-sync are bound together, so updating requirements.txt manually or using pip-sync as a separate tool should be discouraged in my opiion.

[webknjaz]

@ssbarnea I think I only rebased it

[atugushev]

Closed based on https://github.com/jazzband/pip-tools/issues/896#issuecomment-1347488704.