Skip to content

python: always use a venv#40773

Merged
alalazo merged 17 commits intospack:developfrom
haampie:fix/external-python-persistent-venv
May 6, 2024
Merged

python: always use a venv#40773
alalazo merged 17 commits intospack:developfrom
haampie:fix/external-python-persistent-venv

Conversation

@haampie
Copy link
Copy Markdown
Member

@haampie haampie commented Oct 30, 2023

For context of this PR see the last bits of this message, this is the 3rd (?) attempt at fixing issues with Python build isolation and extension install directory layout.

This PR adds a layer of indirection to improve build isolation with and without external Python, as well as usability of environment views.

It adds python-venv as a dependency to all packages that extends("python"), which has the following advantages:

  1. Build isolation: only PYTHONPATH is considered in builds, not user / system packages
  2. Stable install layout: fixes the problem on Debian, RHEL and Fedora where external / system python produces bin/local subdirs in Spack install prefixes. Currently bootstrapping of black, isort, mypy and flake8 is broken on Debian because of this.
  3. Environment views are Python virtual environments (and if you add py-pip things like pip list work)

For a Python package py-app, the structure is as follows:

py-app depends both on python-venv and python, and python-venv depends on python.

On the file system it looks like this:

------------------------------------[python package]-------------------------------------

/opt/py-app/
├── bin
│   └── app                                         # shebang to python-venv python
└── lib
    └── python3.11
        └── site-packages
            └── app
                └── app.py

----------------------------------[python-venv package]----------------------------------

/opt/python-venv/ 
├── bin/
│   ├── activate                                    # venv activate script
│   └── python3.11 -> /opt/python/bin/python3.11    # symlink to underlying Python
└── pyvenv.cfg                                      # venv config file
  
-------------------------------------[python itself]-------------------------------------

/opt/python/
├── bin/
│   └── python3.11
├── include                                         # libraries and headers only in underlying python
├── lib/
│   ├── libpython3.11.so                            # libraries and headers only in underlying python
│   └── python3.11/
│       └── abc.py                                  # Python standard library
└── share

Views with python-venv + python

Views work whether they're symlink, hardlink or copy type (thanks to this PR). They simply flatten the above directory structure:

/view/
├── bin
│   ├── activate                                    # copy of activate script with $VIRTUAL_ENV path rewritten
│   ├── app                                         # shebang rewritten to python in view
│   └── python3.11 -> /opt/python/bin/python3.11    # symlink to underlying Python
├── include
├── lib
│   ├── libpython3.11.so -> /opt/python/lib/libpython3.11.so
│   └── python3.11
│       ├── abc.py -> /opt/python/lib/python3.11/abc.py
│       └── site-packages
│           └── app
│               └── app.py -> /opt/py-app/lib/python3.11/site-packages/app/app.py
├── pyvenv.cfg -> /opt/python-venv/pyvenv.cfg
└── share

which constitutes a Python venv. In case Python is external, you conveniently still get a bin/python3.11 pointing to system Python, which before this PR was not the case.

spec["python"].command

This PR additionally makes spec["python"].command return spec["python-venv"].command. The rationale is that packages in repos we do not own do not pass the underlying python to the build system, which could still result in incorrectly computed install layouts.

Other attributes like libs, headers should be on python anyways and need no change. In the future we can think to make python-venv a "provider wrapper" for a virtual python, and cpython another provider for python (or something along those lines). That would ultimately just clean up internals.

Spack environment views <=> Python venvs

After this PR, the following is a venv equivalent to python3 -m venv ./view:

spack:
  specs: [py-pip]
  view: ./view

The next example is a venv equivalent to python3 -m venv --without-pip ./view:

spack:
  specs: [python-venv]
  view: ./view

However, an environment that just contains python and nothing else is not a venv:

spack:
  specs: [python]
  view: ./view

because there would be no ./view/pyvenv.cfg file. This should in general not be problematic, cause the view also does not contain any Python package that need to be located relative to the view.

Bootstrap environment

Bootstrap logic is simplified with this PR. For convenience, Spack automatically re-concretizes and installs bootstrap environments that do not contain python-venv. That means spack style should just work for users on Debian, no need to spack clean -b.

Possible pitfalls

  1. System Python + system Python packages

    The pyvenv.cfg file contains the following line:

    include-system-site-packages = false

    which means that you cannot import system Python packages with if you marked python external.

    I think this is new behavior. If necessary, we could put a variant on python-venv +system_packages to indicate whether it should include system site packages or not. Such a variant would then also disable build isolation. This PR does not introduce such a variant, I'm just stating this as a workaround if necessary.

Context / Alternatives considered

This all started fixing the underlying problem of #31131 properly s.t. we could install setuptools from sources instead of wheel, so that it would be easier to add patches for its vendored distutils, to fix incorrect rpath flags (we currently apply those patches in python itself, but python 3.12 dropped distutils and recommends distutils from setuptools)

  1. Previously (Improve build isolation in PythonPipBuilder #40224) I tried a temporary virtual environment for the build, that ensured pip could not execute hooks from unrelated user and system packages. This had two problems:
    1. shebangs of generated executables pointed to the venv and had to be patched
    2. the install layout in a venv differs from the install layout of underlying python (e.g. Debian system Python uses bin/local vs bin when run from underlying executable vs venv resp.) since the venv is no longer around, we computed incorrect PYTHONPATHs
  2. Before that I've tried passing -S flags to pip to ensure it doesn't see hooks from user / system packages, but that doesn't really work because pip runs sub-processes that do not inherit these flags.

Closes #42445
Closes #40601
Closes #37632
Closes #35214
Closes #33923
Closes #32769
Closes #32662
Closes #42445
Fixes #31939
Fixes #31131

@spackbot-app

This comment was marked as off-topic.

@haampie haampie force-pushed the fix/external-python-persistent-venv branch 2 times, most recently from 5677a35 to 59b4220 Compare October 30, 2023 13:43
@spackbot-app spackbot-app bot added the documentation Improvements or additions to documentation label Oct 30, 2023
@tgamblin tgamblin added this to the v0.22.0 milestone Oct 30, 2023
@haampie haampie force-pushed the fix/external-python-persistent-venv branch from 9bd04d3 to 1c4301e Compare October 30, 2023 18:54
@haampie haampie changed the title external python: python +external ^system-python Python: revamp the build isolation solution Oct 30, 2023
@haampie haampie marked this pull request as draft October 30, 2023 18:58
@adamjstewart adamjstewart self-assigned this Oct 31, 2023
@haampie haampie force-pushed the fix/external-python-persistent-venv branch from 1c4301e to 4dfb170 Compare November 23, 2023 08:36
@haampie haampie force-pushed the fix/external-python-persistent-venv branch 3 times, most recently from c73e259 to 7ec35ea Compare November 23, 2023 08:41
@haampie haampie force-pushed the fix/external-python-persistent-venv branch from 8a557f7 to 3a02720 Compare May 2, 2024 13:58
@haampie
Copy link
Copy Markdown
Member Author

haampie commented May 3, 2024

Starting to dislike the state of this PR. Making spec["python"].command return spec["python-venv"].command is just a band-aid.

  • Can't fix expressions spec["python"].package.platlib in repos we don't own. In builtin it's used like this:
    os.path.join(spec["dependency"].prefix, spec["python"].package.platlib, ...)
    which is dubious cause it duplicates logic the dependent shouldn't really know; so maybe not a big deal when that breaks if it was shaky to start with? I replaced it with reading the python_platlib global from the dependency.
  • Can't make spec["python"] return spec["python-venv"] because Spack itself uses it in many places.

There sure is "leaky abstractions" of python-venv, but accessing spec["python"].package.* is similar. Imo it's better to let dependencies inform dependents about props so things aren't as tightly coupled as they are now.

@haampie
Copy link
Copy Markdown
Member Author

haampie commented May 3, 2024

And I'm also not convinced that using runtimes for python-venv makes sense at this point in time. We can't express "depend on python-venv under the same conditions and with the same deptype as on python (if it is extended)", both from the perspective of spack dsl (when="^python" is recursive) and asp (needs new rules).

Runtimes are currently rather compiler centric where you don't have that issue, since %gcc is a node prop and has no edge type. Obviously we have to revisit this when going from %gcc to ^gcc, but I'm not comfortable changing runtime logic now. Maybe if the review came a month earlier.

In extends the information is trivially available, and it's just 2 lines.

@haampie haampie force-pushed the fix/external-python-persistent-venv branch from 57ccae2 to c3b131e Compare May 3, 2024 14:33
@haampie haampie requested a review from tgamblin May 4, 2024 09:41
@haampie
Copy link
Copy Markdown
Member Author

haampie commented May 6, 2024

Actually on Fedora we also have issues with install layout, which breaks clingo bootstrapping from sources. Went unnoticed because our GHA shows a false positive due to yet another bug.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 6, 2024

@tgamblin I'm going to merge this PR, as we said on Friday it's basically fine. We need to start rebuilding the release pipeline rather sooner than later. If there are small adjustments to be made, we can have a following PR and backport that on the release branch.

@alalazo alalazo merged commit 125206d into spack:develop May 6, 2024
@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 6, 2024

@tgamblin Your ✔️ appeared on my browser after I hit merge, so I hope you were not working on a commit message 🙂

@jhale
Copy link
Copy Markdown
Contributor

jhale commented Jul 23, 2024

This looks like a welcome change in terms of further isolating Python packages in Spack - but I'm having a few issues!

I am running on the develop branch pulled today. I have a single Python package adios2 which seems to be using python-venv.

Within my Spack environment view I see a folder venv-1.0-7hm6gjnyjywxmsph2fjj45dhgofu6cd4 which contains the adios2 module.

I've tried both bin/python directly and source bin/activate followed by python and import adios2, but it cannot find the module. Note that I can import adios2 manually if I'm in the right directory, so this is definitely a configuration issue.

Here is the pyvenv.cfg within the view:

home = /mnt/lscratch/users/jhale/spack/opt/spack/linux-rhel8-zen2/gcc-13.2.0/python-3.11.7-w6eabydkckjhquqgmrtipec3rozmf7en/bin
include-system-site-packages = false
version = 3.11.7
executable = /mnt/lscratch/users/jhale/spack/opt/spack/linux-rhel8-zen2/gcc-13.2.0/python-3.11.7-w6eabydkckjhquqgmrtipec3rozmf7en/bin/python3.11
command = /mnt/lscratch/users/jhale/spack/opt/spack/linux-rhel8-zen2/gcc-13.2.0/python-3.11.7-w6eabydkckjhquqgmrtipec3rozmf7en/bin/python3.11 -m venv --without-pip /mnt/lscratch/users/jhale/spack/opt/spack/linux-rhel8-zen2/gcc-13.2.0/python-venv-1.0-7hm6gjnyjywxmsph2fjj45dhgofu6cd4

What am I doing wrong?

Edit: All commands are within the activated environment.

spack env st
==> In environment fenicsx-0_8_0-r3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet