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

Try switching to venv for isolation #10720

Closed
wants to merge 7 commits into from

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Dec 9, 2021

I wanna see how difficult this would actually be, by seeing how the CI reacts to this.

Closes #6264, hopefully.
Closes #7953, hopefully.

@pfmoore
Copy link
Member

pfmoore commented Dec 9, 2021

Looks nice 🙂 Am I right that the only changes in src/pip/_internal/distributions/sdist.py are reformattings from black?

@uranusjr
Copy link
Member

uranusjr commented Dec 9, 2021

I was trying to divert most of the work to build’s isolated env refactoring… but this is nice as well :p

@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Dec 10, 2021
@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 10, 2021

Am I right that the only changes in src/pip/_internal/distributions/sdist.py are reformattings from black?

No, I also made some of the arguments kw-only and dropped one of them which might not make sense anymore. That's basically why there's a change in that file.

I'm a bit confused by the normal and overlay prefixing mechanism we've got going on, because it seems like we allowed having pyproject.toml declarations to override whatever is provided by the backend... which... seems wrong?

Anyway... it's past midnight, so... I go to sleep now and come back to this tomorrow morning.

@pradyunsg
Copy link
Member Author

Okay, this builds a wheel locally without any setuptools in the environment (i.e. is using the correct paths for the site-packages in isolation).

❯ pip list | grep setuptools
❯ pip wheel ./tests/data/src/withpyproject 
Processing ./tests/data/src/withpyproject
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: withpyproject
  Building wheel for withpyproject (pyproject.toml) ... done
  Created wheel for withpyproject: filename=withpyproject-0.0.1-py3-none-any.whl size=1033 sha256=099603760ff65c5b1ad898100b828cf492b70a60b047a5ac739ae08c4e2ad3ef
  Stored in directory: /private/var/folders/y1/j465wvf92vs938kmgqh63bj80000gn/T/pip-ephem-wheel-cache-68k37ath/wheels/0b/84/c7/bd6357391f2f15389e0162876f3242eb89a643b92bfb96721a
Successfully built withpyproject

Let's see what the test suite says. :)

@pradyunsg pradyunsg added C: build logic Stuff related to metadata generation / wheel generation type: enhancement Improvements to functionality and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels Dec 10, 2021
@pradyunsg
Copy link
Member Author

FAILED tests/functional/test_build_env.py::test_build_env_requirements_check
FAILED tests/functional/test_build_env.py::test_build_env_isolation - Asserti...
FAILED tests/functional/test_install.py::test_pep518_refuses_conflicting_requires
FAILED tests/functional/test_install.py::test_editable_install__local_dir_no_setup_py_with_pyproject
FAILED tests/functional/test_pep517.py::test_backend - AssertionError: assert...
FAILED tests/functional/test_pep517.py::test_conflicting_pep517_backend_requirements
FAILED tests/functional/test_pep517.py::test_pep517_backend_requirements_already_satisfied

Alright, not gonna lie, this is better than I expected.

@pradyunsg
Copy link
Member Author

So, I've spent a decent chunk of time today picking this back up. I think the current logic that we have for build environments in main is... subtly wrong?

It currently has two "prefixes": normal and overlay. Packages installed in overlay are preferred over what is specified in normal. This mechanism is used to separate the two ways we can get dependencies during a build:

  • overlay contains the dependencies declared in pyproject.toml.
  • normal contains the dependencies declared via get_requires_for_*.

This is fairly weird, because I think we should be erroring out cleanly if there's any sort of inconsistency between the two sets of dependency declarations, instead of handling these in the way that we're currently handling them.

@uranusjr
Copy link
Member

Yeah I think we should. And even if we don’t, it seems the logic should have been entirely the other way around; I don’t see how requirements from pyproject.toml should be preferred over those from PEP 517 hooks.

Since there are very unlikely any libraries relying on this behaviour (and they shouldn’t anyway) because we do emit warnings when there are unsatisfied dependencies, let’s just change that.

This should be logically equivalent, and does not require understanding
of ExitStack to understand what it's doing.
This allows the caller to control which executable is used, allowing
for the caller to tweak the installation semantics.
This makes the call-site easier to read, since the caller explicitly
specifies what argument corresponds to the requirements to be installed.
This helps ensure that the user gets a proper build environment.
This ensures that the string is not highlighted as a regular expression,
on editors that do that (eg: Visual Studio Code).
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 17, 2022
@pradyunsg
Copy link
Member Author

Thinking about this a bit more, I think this needs a slightly different approach.

This should roll out via our transitioning model, via --use-feature and --use-deprecated flags. I'll break this PR up into multiple chunks, to make this easier to roll this out eventually.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jul 15, 2022
@d1saster
Copy link

d1saster commented Sep 4, 2022

Sry for posting this here, but this is the place that comes closest to discussing future imporvements in build_env.py.

What I feel lacking currently in build_env.py is that it does not at all take into account InstallRequirements down the road. I would expect that most packages assume to find the respective required package during build time in the same version as it will later installed alongside.

In helmholtz-analytics/mpi4torch#7 I am currently running into exactly this issue. E.g. executing pip install -v mpi4torch==0.1.1 torch=1.11.0 results with a cached version of torch==1.12.1 into the following (you need a working MPI install for this to work)

Using pip 22.3.dev0 from /home/xyz/src/mpi4torch_env/pip/src/pip (python 3.9)
Collecting mpi4torch==0.1.1
  Downloading mpi4torch-0.1.1.tar.gz (58 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 58.7/58.7 kB 539.2 kB/s eta 0:00:00
  Running command pip subprocess to install build dependencies
  Ignoring importlib-metadata: markers 'python_version < "3.8"' don't match your environment
  Collecting torch>=1.9.0
    Using cached torch-1.12.1-cp39-cp39-manylinux1_x86_64.whl (776.4 MB)
  Collecting setuptools>=40.8.0
    Using cached setuptools-65.3.0-py3-none-any.whl (1.2 MB)
  Collecting wheel
    Using cached wheel-0.37.1-py2.py3-none-any.whl (35 kB)
  Collecting typing-extensions
    Using cached typing_extensions-4.3.0-py3-none-any.whl (25 kB)
  Installing collected packages: wheel, typing-extensions, setuptools, torch
  Successfully installed setuptools-65.3.0 torch-1.12.1 typing-extensions-4.3.0 wheel-0.37.1
  Installing build dependencies ... done
  Running command Getting requirements to build wheel
  running egg_info
  writing mpi4torch.egg-info/PKG-INFO
  writing dependency_links to mpi4torch.egg-info/dependency_links.txt
  writing requirements to mpi4torch.egg-info/requires.txt
  writing top-level names to mpi4torch.egg-info/top_level.txt
  reading manifest file 'mpi4torch.egg-info/SOURCES.txt'
  reading manifest template 'MANIFEST.in'
  adding license file 'LICENSE'
  writing manifest file 'mpi4torch.egg-info/SOURCES.txt'
  Getting requirements to build wheel ... done
  Running command Preparing metadata (pyproject.toml)
  running dist_info
  creating /tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info
  writing /tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/PKG-INFO
  writing dependency_links to /tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/dependency_links.txt
  writing requirements to /tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/requires.txt
  writing top-level names to /tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/top_level.txt
  writing manifest file '/tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/SOURCES.txt'
  reading manifest file '/tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/SOURCES.txt'
  reading manifest template 'MANIFEST.in'
  adding license file 'LICENSE'
  writing manifest file '/tmp/pip-modern-metadata-sgo74axg/mpi4torch.egg-info/SOURCES.txt'
  creating '/tmp/pip-modern-metadata-sgo74axg/mpi4torch-0.1.1.dist-info'
  adding license file "LICENSE" (matched pattern "LICEN[CS]E*")
  Preparing metadata (pyproject.toml) ... done
Collecting torch==1.11.0
  Using cached torch-1.11.0-cp39-cp39-manylinux1_x86_64.whl (750.6 MB)
ERROR: Cannot install mpi4torch==0.1.1 and torch==1.11.0 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested torch==1.11.0
    mpi4torch 0.1.1 depends on torch==1.12.1

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

I would expect pip to respect these package versions as constraints during setup of the build environment.

@pradyunsg
Copy link
Member Author

That’s somewhat inherent to build-isolation, can be controlled to an extent via PIP_CONSTRAINTS, is unrelated to this PR, has been discussed in various places in the past (eg: see oldest-supported-numpy for example) and is strongly coupled with the idea that you should be able to deal with all kinds of build-time complexities within a single pip install call.

I don’t have links handy, and I don’t wanna load this PR with a discussion that’s unrelated to it; please look at the existing closed/locked issues about this or file a new feature request if you think there’s more to discuss on this topic.

@d1saster
Copy link

d1saster commented Sep 4, 2022

Agreed. Thx for the oldest-supported-numpy pointer.

@pradyunsg
Copy link
Member Author

Closing in favor of #11619

@pradyunsg pradyunsg closed this Dec 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: build logic Stuff related to metadata generation / wheel generation needs rebase or merge PR has conflicts with current master type: enhancement Improvements to functionality
Projects
None yet
5 participants