Skip to content

sdh_pip_install: Force reinstallation of built wheels; add sage_setup source deps #32659

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

Closed
mkoeppe opened this issue Oct 9, 2021 · 28 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 9, 2021

After #32492, we may get

[sage_conf-9.5.beta2] sage-conf is already installed with the same version as the provided wheel. Use --force-reinstall to force an installation of the wheel.

Because of this behavior of pip, some configuration changes made in configure will not be updated in an incremental build. Likewise for other Python packages when we add a patch, or for sage-setup when we edit files there (as happened in #32607).

However, as discussed in pypa/pip#8711, the pip install option --force-reinstall does too much -- it also reinstalls all dependencies, which we definitely do not want.

Instead, we just use pip uninstall before pip install.

We also add some dependencies on source files to sage_setup. This is also for #32607.

CC: @jhpalmieri @vbraun @yyyyx4

Component: build

Author: Matthias Koeppe

Branch: d435781

Reviewer: Lorenz Panny

Issue created by migration from https://trac.sagemath.org/ticket/32659

@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 9, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

comment:1

A solution provided by pypa/pip#9147 (was merged in pip 20.3) is to use a specification sage-conf @ file:///..... (see pypa/pip#8711 (comment)), which will ensure reinstallation.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

Commit: 5855e54

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

comment:3

Unfortunately this does not fix the issue:

[sage_setup-9.5.beta2] Processing /Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/wheels/sage_setup-9.5b2-py3-none-any.whl
[sage_setup-9.5.beta2] Requirement already satisfied: pkgconfig in /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages (from sage_setup@ file:///Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/wheels/sage_setup-9.5b2-py3-none-any.whl) (1.5.5)
[sage_setup-9.5.beta2] sage-setup is already installed with the same version as the provided wheel. Use --force-reinstall to force an installation of the wheel.

New commits:

5855e54build/bin/sage-pip-install: Use pip install "distribution @ file:///wheelfile"

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

comment:4

This behavior of pip is subject to current discussion/development -

Un-deprecate source distribution re-installation behaviour
pypa/pip#10543

Rewrite direct URL reinstallation logic by uranusjr
pypa/pip#10564

Updating remote links with new URLs for PEP508 functionallity
pypa/pip#5780

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2021

comment:5

Another possible workaround is proposed in pypa/pip#8711 (comment)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

9c620c4build/bin/sage-pip-install: Add a query suffix to the file:/// URL in an attempt to force reinstallation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Changed commit from 5855e54 to 9c620c4

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 10, 2021

comment:7

Unfortunately also that does not fix the issue:

[sage_setup-9.5.beta2] Requirement already satisfied: pkgconfig in /Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.9/site-packages (from sage_setup@ file:///Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/wheels/sage_setup-9.5b2-py3-none-any.whl?hash=sha256:285343dcfde7ad2e51f630591faba468aad4875ee0f15398346642092a8e842b) (1.5.5)
[sage_setup-9.5.beta2] sage-setup is already installed with the same version as the provided wheel. Use --force-reinstall to force an installation of the wheel.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 10, 2021

comment:8

Also another approach, providing a --hash (via a requirements file) fails.

diff --git a/build/bin/sage-dist-helpers b/build/bin/sage-dist-helpers
index 4a8862d50f..e7b8fc8677 100644
--- a/build/bin/sage-dist-helpers
+++ b/build/bin/sage-dist-helpers
@@ -271,7 +271,20 @@ sdh_store_wheel() {
     mkdir -p "${SAGE_DESTDIR}${SAGE_SPKG_WHEELS}" && \
         $sudo mv "$wheel" "${SAGE_DESTDIR}${SAGE_SPKG_WHEELS}/" || \
         sdh_die "Error storing $wheel"
+    wheel_basename="${wheel##*/}"
     wheel="${SAGE_DESTDIR}${SAGE_SPKG_WHEELS}/${wheel##*/}"
+    # Trac #32659: pip no longer reinstalls local wheels if the version is the same.
+    # Because neither (1) applying patches nor (2) local changes (in the case
+    # of sage-conf, sage-setup, etc.) bump the version number, we need to
+    # override this behavior.  The pip install option --force-reinstall does too
+    # much -- it also reinstalls all dependencies, which we do not want.
+    # We override it by specifying the hash -- but this can only be done using a
+    # requirements file, not using flags on the command line,
+    # https://github.com/pypa/pip/issues/3257
+    distname="${wheel_basename%%-*}"
+    hasharg=$(python3 -m pip hash "$wheel" | sed -n /--hash=/p)
+    requirements_file="${SAGE_DESTDIR}${SAGE_SPKG_WHEELS}/requirements-$distname.txt"
+    echo "$distname @ file://$wheel $hasharg" | $sudo tee $requirements_file > /dev/null
 }
 
 sdh_store_and_pip_install_wheel() {
@@ -306,7 +319,7 @@ sdh_store_and_pip_install_wheel() {
         local sudo=""
         local root=""
     fi
-    $sudo sage-pip-install $root $pip_options "$wheel" || \
+    $sudo sage-pip-install $root $pip_options -v -v -v -r "$requirements_file" || \
         sdh_die "Error installing ${wheel##*/}"
     if [ -n "${SAGE_PKG_DIR}" ]; then
         # Record name of installed distribution name for uninstallation.

It still says:

[sage_setup-9.5.beta2] sage-setup is already installed with the same version as the provided wheel. Use --force-reinstall to force an installation of the wheel.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 10, 2021

comment:9

I think we will need either pypa/pip#10564 (and use --upgrade with pip install); or go back to pip uninstall before pip install (a simple version of what was removed in #31816).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Changed commit from 9c620c4 to b5a89ad

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2d852adbuild/bin/sage-dist-helpers (sdh_pip_uninstall): Use build/bin/sage-pip-uninstall, resurrected in simplified form
b5a89adbuild/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Uninstall before installing, to ensure the wheel is installed even if the version is the same

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Changed commit from b5a89ad to 30a272e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

30a272ebuild/pkgs/sage_setup/dependencies: Add interpreter specs as dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 10, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title sdh_pip_install: Force reinstallation of built wheels sdh_pip_install: Force reinstallation of built wheels; add sage_setup source deps Oct 10, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

comment:14

A new pip version is out (#32671); it makes some changes closely related to this issue but does not solve anything.

Needs review

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 18, 2021

comment:15

Is it intended that sdh_store_and_pip_install_wheel invokes python3 -m pip uninstall rather than sage-pip-uninstall?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2021

Changed commit from 30a272e to d435781

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

d435781build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Use sage-pip-uninstall

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 18, 2021

comment:17

Looks like I introduced sage-pip-uninstall to avoid a duplication but then forgot to use it in one of the two cases

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 18, 2021

comment:18

Thanks, looks good to me.

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 18, 2021

Reviewer: Lorenz Panny

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 18, 2021

comment:19

Thanks for reviewing!

@vbraun
Copy link
Member

vbraun commented Oct 20, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2022

Changed commit from d435781 to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2022

Replying to [ticket:32659 Matthias Köppe]:

However, as discussed in pypa/pip#8711, the pip install option --force-reinstall does too much -- it also reinstalls all dependencies, which we definitely do not want.

This discussion is still ongoing and has moved to pypa/pip#10564 (most recent activity: June 2022)

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

No branches or pull requests

3 participants