Skip to content

Normal Python packages: postpone wheel installation to the post-install phase #36743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 10, 2023
Merged

Normal Python packages: postpone wheel installation to the post-install phase #36743

merged 5 commits into from
Dec 10, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Nov 21, 2023 •
edited
Loading

Previously, on sdh_pip_install, the wheel file is staged in DESTDIR, but the wheel is installed immediately.
Now we store a new script spkg-pipinst, which is run after unloading DESTDIR (and before any spkg-postinst script), which does the actual installation of the wheel.

Apart from this and some changes to the messages displayed during package installation, this should make no difference for any of our packages.

Just so that it is tested for at least one package in CI, we include a small package update.

Together with

this is preparation for requiring only the build dependencies ("build-system requires") while building a wheel for the package, and to require the runtime dependencies ("install-requires") only later, when the wheel is to be installed.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@jhpalmieri
Copy link
Member

pip failed to build:

real	0m0.751s
user	0m0.370s
sys	0m0.253s
Copying package files from temporary location /Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/var/tmp/sage/build/pip-23.3.1/inst to /Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11
Running pip-install script for pip-23.3.1.
/Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
(ignoring error)
/Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
Warning: installing with "python3 -m pip install --verbose --no-index --find-links=/Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/var/lib/sage/wheels --disable-pip-version-check --isolated --no-cache-dir" failed. Retrying, adding "--no-deps --ignore-installed --ignore-requires-python"
/Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
Error: installing with pip  failed
********************************************************************************
Error installing --ignore-installed -r
/Users/jpalmier/Sage/TESTING/sage-10.2.rc4/local/var/lib/sage/venv-python3.11/var/lib/sage/scripts/pip/spkg-requirements.txt
********************************************************************************
************************************************************************
Error running the pipinst script for pip-23.3.1.
************************************************************************

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2023

In hindsight it's obvious that I should have tested this case...

@jhpalmieri
Copy link
Member

jhpalmieri commented Dec 1, 2023 •
edited
Loading

I don't typically set SAGE_SUDO, but to test I set it to 'sudo -E' and once again, pip failed to install:

removing /var/folders/7c/bz8vl5tx47ndyy8knwrtg1k00000gp/T/tmp.Y4ofb1qD0R
/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
(ignoring error)
/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
Warning: installing with "python3 -m pip install --verbose --no-index --find-links=/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/lib/sage/wheels --disable-pip-version-check --isolated --no-cache-dir" failed. Retrying, adding "--no-deps --ignore-installed --ignore-requires-python"
/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/bin/python3: No module named pip
Error: installing with pip  failed
********************************************************************************
Error installing
--root=/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/tmp/sage/build/pip-23.3.1/inst
--no-warn-script-location --ignore-installed -r
/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/lib/sage/scripts/pip/spkg-requirements.txt

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 3, 2023

Documentation preview for this PR (built with commit f981b15; changes) is ready! 🎉

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 3, 2023

I don't typically set SAGE_SUDO, but to test I set it to 'sudo -E' and once again, pip failed to install

Thanks for testing! Fixed now.

@jhpalmieri
Copy link
Member

jhpalmieri commented Dec 4, 2023 •
edited
Loading

Maybe things with SAGE_SUDO="sudo -E" still aren't right. With this branch but without setting SAGE_SUDO, just building normally, ./sage --pip freeze | grep tmp returns no hits, but with SAGE_SUDO="sudo -E", I get lots of hits:

% ./sage --pip freeze | grep tmp | wc
     140     420   50093

This looks like the same as building without this branch. Examples with this branch + setting SAGE_SUDO:

alabaster @ file:///Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/tmp/sage/build/alabaster-0.7.12/inst/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/lib/sage/wheels/alabaster-0.7.12-py2.py3-none-any.whl#sha256=97a89e510af23283d07834c7760e08086d4f8813597e860b6f855ff3c09e7863
appnope @ file:///Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/tmp/sage/build/appnope-0.1.3/inst/Users/jpalmier/Sage/TESTING/test/sage-10.2.rc5/local/var/lib/sage/venv-python3.11/var/lib/sage/wheels/appnope-0.1.3-py2.py3-none-any.whl#sha256=3a530d0a94cfac4ba3187c16266dbb3d338216726a374c94dcaeb999cfe68b37

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2023

You are right. I do not have a solution for fixing wheel URL for the SAGE_SUDO case. I have updated the PR description.

@jhpalmieri
Copy link
Member

Okay. As far as I can tell, this doesn't make anything worse (or even really change anything) if SAGE_SUDO is set, and it is an improvement if SAGE_SUDO is not set. Let's merge it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2023

Thank you!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2023

At some point we may want to deprecate SAGE_SUDO. Introducing it was helpful as it informed restructuring the installation of packages into build vs. install phase. But my own use case for this feature (for use by IT on a shared installation) has gone away, and I'm not sure if this is used by others.

@vbraun vbraun merged commit abb1a51 into sagemath:develop Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhpalmieri jhpalmieri jhpalmieri approved these changes

@orlitzky orlitzky Awaiting requested review from orlitzky

Assignees

@mkoeppe mkoeppe

Labels
c: build c: packages: standard
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

sdh_store_and_pip_install_wheel: Install from the persistent wheel directory
3 participants
@mkoeppe @jhpalmieri @vbraun