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

pip-sync yields different results depending on pre-installed package set #896

Closed
AndydeCleyre opened this issue Sep 15, 2019 · 22 comments
Closed
Labels
feature Request for a new feature needs discussion Need some more discussion

Comments

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Sep 15, 2019

Update: The shape of this issue has changed a bit since creation, due to discoveries in this discussion, and the addition of the --pip-args option. I'm renaming it, and adding a new summary:


If pip-sync has to actively install a requirement, its dependencies will end up in the environment. But if the requirement is already installed when pip-sync runs, its dependencies not present in the txt will be removed, "breaking" the environment.

$ 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

A txt which omits dependencies is, by pip-tools standards, malformed, and therefore pip-sync's behavior is not currently guaranteed or defined. But:

  1. As @Zac-HD says below:

IMO "running pip-sync a second time makes no further changes" is a very important property to maintain

  1. IMO a package is not "properly" installed if its dependencies aren't present, and pip-sync should achieve "proper" installs.

  2. If a user wants to "break" dependencies, they can explicitly do so with --pip-args --no-deps

  3. While I don't see an advantage to the current (inconsistent) behavior, having the deps always installed (unless --no-deps is specified) would enable users to use pip-sync without issue on txts that don't come from pip-compile. Though not officially supported, it's a nice bonus to be able to use pip-sync uniformly as a complete replacement/wrapper for pip install -r. Now that Exclude requirements with non-matching markers from pip-sync's merge stage #927 is merged, I don't know of any other obstacle to supporting "foreign" txts (in practice, if not policy).

My idea of a solution is #907, which keeps already-installed packages in the to-install set, as pip itself will refrain from reinstalling those anyway, and will ensure its dependencies get installed if necessary.


Original report:

I looked but didn't find an existing issue for this.

What's the problem this feature will solve?

I would like to use pip-sync on requirements.txt files that are not generated by pip-compile, for arbitrary (other folks') Python projects. In fact I had been doing so, and I guess I have been getting lucky, as I only recently came to a case where this failed, and was surprised to find when re-reading pip-tools's README:

Be careful: pip-sync is meant to be used only with a requirements.txt generated by pip-compile

The failure I notice is that pip-sync will uninstall hard dependencies of packages listed in the requirements.txt. I don't know if there are further problems.

Describe the solution you'd like

Until/unless there are other specifically identified problems for externally crafted requirements.txt files, the solution would seem to be for pip-sync to refrain from uninstalling dependencies of explicitly listed packages. This might be as simple as doing all the uninstalling before any of the installing.

This would enable pip-tools users to continue to use pip-sync across all Python projects they work on, even if they are not in a position to enforce use of pip-compile to manage those projects' requirements.txt generation.

Alternative Solutions

Alternatively, of course, users can just use pip install -r requirements.txt. That however does not have the advantage of stripping the environment of non-required packages. Although I guess the solution could then be as simple as pip-sync requirements.txt && pip install -r requirements.txt.

Additional context

Simple demonstration of the problem:

echo "pytest==5.1.2" > requirements.txt
pip install -r requirements.txt
pip freeze
atomicwrites==1.3.0
attrs==19.1.0
Click==7.0
importlib-metadata==0.22
more-itertools==7.2.0
packaging==19.1
pip-tools==4.1.0
pluggy==0.13.0
py==1.8.0
pyparsing==2.4.2
pytest==5.1.2
six==1.12.0
wcwidth==0.1.7
zipp==0.6.0
pip-sync requirements.txt
pip freeze
Click==7.0
pip-tools==4.1.0
pytest==5.1.2
six==1.12.0

The result is a broken installation of pytest.

@atugushev atugushev added feature Request for a new feature needs discussion Need some more discussion labels Sep 17, 2019
@AndydeCleyre

This comment has been minimized.

@ulope
Copy link
Member

ulope commented Sep 23, 2019

@AndydeCleyre I assume you pinged me because of my thumbs-down vote.
I'm just a user of pip-tools so my vote has no more weight than anyone else's, but to spell out why I think this is a bad idea:

What you request would be a complete reversal of how pip-tools works. All the 'smarts' are in pip-compile. pip-sync just ensures that the installed packages exactly match the requirements file. It seems unwise to change this.

Maybe it would be possible for your case to pass your initial (third party generated) requirements.txt through pip-compile first and that way get a pip-sync compatible output.

@AndydeCleyre
Copy link
Contributor Author

Thanks, yes, I wanted to encourage discussion here.

On the other side, I'd say that installing a requirement's dependencies is part of installing that requirement, and that users should expect any installed package to have its dependencies also installed.

I took a look and saw that piptools/sync.py:sync already does the uninstalling before the installing, which I thought would be a solution to this issue. It turns out the current behavior is more subtle and less predictable:

echo "pytest==5.1.2" > requirements.txt
pip install -r requirements.txt
pip-sync requirements.txt
# pytest remains installed but broken, with dependencies removed
pip-sync /dev/null
pip-sync requirements.txt
# pytest is installed "properly," with its dependencies included

In other words, if pip-sync has to actively install a requirement, its dependencies will end up in the environment. But if the requirement is already installed, its dependencies not present in the txt will be removed, breaking the environment.

Here the inconsistency is more simply demonstrated:

echo "pytest==5.1.2" > requirements.txt
pip-sync /dev/null
pip-sync requirements.txt
pip freeze | wc -l
# 14
pip-sync requirements.txt
pip freeze | wc -l
# 4

So the environment state that pip-sync will achieve depends not only on the requirements files, but also the state of the environment at time of operation. I don't think this is desirable or predictable.

While my position is for not breaking dependencies, either way the project goes on that, I think we can agree that it should behave more consistently in this case.

I will submit a pull request for my idea of a fix for this, though I understand that it will likely just be an anchor for continued discussion of "correct" behavior here.

@Zac-HD
Copy link

Zac-HD commented Oct 4, 2019

pass your initial (third party generated) requirements.txt through pip-compile first and that way get a pip-sync compatible output.

That's my preferred solution!

Perhaps pip-sync could also use pip's --no-deps option to avoid installing non-specified dependencies? That way you'd get the same result regardless of what was installed initially.

IMO "running pip-sync a second time makes no further changes" is a very important property to maintain, while in this case it wouldn't even converge to a stable point.

@atugushev
Copy link
Member

Perhaps pip-sync could also use pip's --no-deps option to avoid installing non-specified dependencies? That way you'd get the same result regardless of what was installed initially.

That's an interesting idea!

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre
Copy link
Contributor Author

@Zac-HD @ulope

What do you think of #907? It brings consistent behavior where we currently have surprises, and it brings in --no-deps for when you want to break your dependencies.

I don't think --no-deps or default-implied "yes-deps" is out of scope for pip-sync as pip-sync is largely analogous to pip install, which behaves that way.

@AndydeCleyre

This comment has been minimized.

@AndydeCleyre
Copy link
Contributor Author

@ulope What do you think? In order to fix the inconsistent behavior we'll have to change what pip-sync does one way or another. #907 gives us the choice of --no-deps or not, just like pip install.

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Apr 12, 2020

Alright, I've rebased #907 onto master, now that we've got --pip-args. With #907, here's what the consistent behavior looks like:

$ 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

And here's the behavior in current master, without #907:

$ 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

This shows that --pip-args --no-deps allows us to achieve consistent behavior if --no-deps behavior is desired, and that #907 will offer consistent behavior if you want installed packages to have their dependencies installed.

Please have a look and let me know what you think, @atugushev @Zac-HD @Minkey27 @ulope

@AndydeCleyre AndydeCleyre changed the title Support requirements.txt files not generated by pip-compile, by not uninstalling dependencies pip-sync yields different results depending on pre-installed package set Apr 15, 2020
@ulope
Copy link
Member

ulope commented Apr 16, 2020

Unfortunately my opinion hasn't changed from when I made my last comment in this issue.

For me the whole point of using pip-tools is that the generated requirements.txt exactly describes what the installed virtualenv will look like afterwards.

I don't think it's a good idea to change the default (which would also be backwards incompatible as well) and force everyone wanting to retain the current behaviour to pass --pip-args --no-deps in the future.

As for the inconsistency you note in #907 (12 vs 4 packages installed), that seems like a bug to me. It should always behave in the no-deps way and only install 4 packages.

So TL;DR:
I'm not at all against providing the "include deps" behaviour as an option but I'm very much against making it the default.

@AndydeCleyre
Copy link
Contributor Author

I appreciate the feedback @ulope, thanks!

@AndydeCleyre
Copy link
Contributor Author

A year and a half since opening, and I'm still curious if anyone has encountered a workflow that would be broken by #907.

I believe some are concerned that existing workflows would be disrupted, but AFAICT that is untrue.

There may be good reason to reject this change, so let's not muddy the waters with bugaboos.

AndydeCleyre added a commit to AndydeCleyre/pip-tools that referenced this issue Nov 9, 2021
…g sync.

By including already-installed matching requirements in the to_install set,
dependencies of pinned packages will be installed (after potentially being uninstalled),
regardless of environment state at time of sync, for a more consistent, predictable,
and practical resulting state. Fixes jazzband#896.

Modify tests to expect explicit requirements in the to_install set, to match.
AndydeCleyre added a commit to AndydeCleyre/pip-tools that referenced this issue Feb 8, 2022
…g sync.

By including already-installed matching requirements in the to_install set,
dependencies of pinned packages will be installed (after potentially being uninstalled),
regardless of environment state at time of sync, for a more consistent, predictable,
and practical resulting state. Fixes jazzband#896.

Modify tests to expect explicit requirements in the to_install set, to match.
webknjaz pushed a commit to AndydeCleyre/pip-tools that referenced this issue Oct 5, 2022
Fixes jazzband#896

By including already-installed matching requirements in the to_install set,
dependencies of pinned packages will be installed,
after potentially being uninstalled,
regardless of environment state at time of sync.

The already-installed reqs will still not be reinstalled,
thanks to pip's own handling.

The resulting state is more consistent, predictable, and practical.

Modify tests to expect explicit requirements in the to_install set, to match.
@atugushev
Copy link
Member

Well, It's been some time so I will close this issue now (and PR) since it didn't get positive feedback (3 nays vs 1 yay). Feel free to reopen if you find broken workflows in a wild due to the issue. Sorry!

@atugushev atugushev closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Request for a new feature needs discussion Need some more discussion
Projects
None yet
5 participants