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

Run pip-compile in env python rather than pip-tools python #1998

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

georgek
Copy link
Contributor

@georgek georgek commented Oct 7, 2023

Resolves #1087

Usually pip-tools is installed inside a project env and run from there. So it gets run in the same version of Python that the project is developed with. This doesn't work if pip-tools is installed somewhere else, like when installed with pipx.

The change here detects whether the currently running Python is different to the one on the PATH. Such a mismatch would probably always lead to unexpected behaviour. So in that case it creates a temporary venv with the Env Python, installs pip-tools, and runs it again there.

This approach could also be modified to allow a python_executable to be passed explicitly (and falling back to PATH by default), similar to how pip-sync is now. One could generated requirements file for multiple versions of Python (of course, you could already do it with tox, but this gives another option).

There are still some TODOs:

  • Add similar behaviour to pip-sync (it already takes python_executable option, but can use env Python if not given),
  • Improve the tests. It currently mocks the subprocess calls but doesn't check the calls are correct (in particular the passed env should be checked probably).

Finally, this is technically a breaking change. It's intended to resolve the pipx scenario (or even installed with OS package manager). I can't think of any reason why anyone would want pip-tools to run on an interpreter other than the one in the current env, but it will probably still break someone's workflow...

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@chrysle chrysle added feature Request for a new feature backwards incompatible Backwards incompatible change labels Oct 8, 2023
@chrysle
Copy link
Contributor

chrysle commented Oct 9, 2023

I just remembered, I think it could be considered avoiding a python-specific virtual environment employing the feature proposed in #1898. What do you think?

@georgek
Copy link
Contributor Author

georgek commented Oct 9, 2023

OK, so a few things here:

It's not possible to run env_python -m piptools ... or something like that because piptools is not (necessarily) installed env_python. There is, of course, PYTHONPATH fuckery, but that causes all kinds of problems usually (as current_python and env_python are potentially different versions of Python)

We just essentially need pip-tools installed in env_python, and a venv is the best way to do that. So that's why I'm making my own venv.

I did consider leveraging build as this also deals with creating temporary venvs (in fact, the code in the PR is essentially what build does). The venv build makes is for its own purposes, though (it installed build tools and stuff). But of course we could still call build to create our own venv, however it doesn't seem to support creating a venv for another interpreter. The only way seems to be to launch a subprocess with the other interpreter (but I might have missed something). It would certainly be nicer to not have to call a subprocess to create the venv.

Regarding #1898 : That could indeed be a better solution. I wasn't aware pip could be used like this. So essentially if we detect the version mismatch we can supply --python-version to match env_python. Or perhaps the --python option can be used instead to avoid having to query env_python for its version at all?

@chrysle
Copy link
Contributor

chrysle commented Oct 9, 2023

Or perhaps the --python option can be used instead to avoid having to query env_python for its version at all?

Yes, sounds good! I'll try to get that pull request merged soon so you can rebase.

@georgek
Copy link
Contributor Author

georgek commented Oct 9, 2023

One question is what to do with the version in the output header. With the change in #1898 we could get a header like:

# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
#    pip-compile --pip-args='--python=python3.8'

Which is OK I suppose because the python version is explicit (even if the Python 3.11 bit is now irrelevant).

But if we change it automatically I suppose we should change that header to say it was run with the env_python version?

@chrysle
Copy link
Contributor

chrysle commented Oct 10, 2023

Well, the header is generated in the writer package's following passage:

def write_header(self) -> Iterator[str]:
if self.emit_header:
yield comment("#")
yield comment(
"# This file is autogenerated by pip-compile with Python "
f"{sys.version_info.major}.{sys.version_info.minor}"
)

We could pass a reference to the target python to the output writer in the compile script by a new parameter, instead of requesting the sys version info (though this'd require making cli() and cli_runner() one again, which I think is the better solution).

@georgek georgek marked this pull request as draft October 10, 2023 16:05
@georgek
Copy link
Contributor Author

georgek commented Oct 10, 2023

Yes, I think given that pip can essentially do what we need now (since last year it seems) the solution in this PR is no longer necessary. I will rewrite it soon. I do wonder if we can get the Python version without resorting to a subprocess call ourselves, though. Perhaps it can be queried via pip.

@chrysle
Copy link
Contributor

chrysle commented May 6, 2024

@georgek I'm inclined to revive the PR at its current state, as it seems there is no other possibility right now. Efforts to decouple from pip's internals might be taken in the future, which would give us the ability to rework the logic. Could you fix the merge conflicts?

@chrysle chrysle added needs rebase Need to rebase or merge awaiting response Awaiting response from a contributor labels May 12, 2024
@chrysle
Copy link
Contributor

chrysle commented May 28, 2024

CI is unfortunately failing on Windows right now, and I don't run that...

@georgek
Copy link
Contributor Author

georgek commented May 29, 2024

Me neither. I don't have much time to work on this right now, unfortunately, but it's on my todo list. If nobody else can figure it out first I'll try to find a Windows box to test it on at some point.

@chrysle chrysle removed needs rebase Need to rebase or merge awaiting response Awaiting response from a contributor labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible Backwards incompatible change feature Request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for installation via pipx
2 participants