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

sage-pip-install: Try installing without --no-deps first #32492

Closed
mkoeppe opened this issue Sep 8, 2021 · 38 comments
Closed

sage-pip-install: Try installing without --no-deps first #32492

mkoeppe opened this issue Sep 8, 2021 · 38 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 8, 2021

We currently use pip install with the option --no-deps - which suppresses installation of runtime dependencies, as we rely on our own dependency management.

In order to make package upgrades (which might change dependencies) more robust, we change this to first attempt an install without --no-deps. Since we still use --no-index (we also throw in a --no-cache in this ticket), it will not actually attempt to install from PyPI but fail with an error.
We then issue a warning (for developers) but fall back to the install with --no-deps (for robustness for users).

(Prompted by #31280 comment:93, #30974 comment:14)

We also pass --ignore-requires-python in the wheel-building phase and in the retry. This simplifies the Python 3.10 preparation and is otherwise harmless.

To test:

eval ./configure $(./config.status --config) --with-sage-venv=`pwd`/venv-32492
make build 
grep -B2 -r "Warning.*try.*no-deps" logs/pkgs

(this will rebuild all Python packages in a new venv)

In particular highly parallel builds reveal a few missing dependencies -- but there should be no new errors.

Fixing the dependencies will be done in follow-up ticket #32493, not here.

Depends on #29847
Depends on #32361

CC: @dimpase @seblabbe @jhpalmieri

Component: build

Author: Matthias Koeppe

Branch/Commit: 92c855c

Reviewer: John Palmieri

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

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

mkoeppe commented Sep 8, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

New commits:

0c387cbbuild/bin/sage-pip-install: Try installing without --no-deps first

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

Commit: 0c387cb

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Changed commit from 0c387cb to 4a34573

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

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

4a34573build/bin/sage-pip-install: Try installing without --no-deps first

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Changed commit from 4a34573 to 6f1c68e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

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

6f1c68esrc/doc/en/developer/packaging.rst: Update

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

comment:10

Adding #29847 as a dependency, which fixes the venv install for memory_allocator

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

Dependencies: #29847

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

9aa1f10Merge #29585
6b1b222Merge #32046
a41b507Merge branch 't/32046/use_pip___use_feature_in_tree_build__replace_use_of_sdh_setup_bdist_wheel_by_sdh_pip_install' into t/29847/install_sage_setup_with_pip__move_sage_include_directories__cython_aliases_from_sage_env_to_sage_setup
478ffcdMerge #32046
b2368c6pkgs/sage-setup/setup.cfg: Add pkgconfig as install_requires
db8e30dMerge tag '9.5.beta0' into t/29847/install_sage_setup_with_pip__move_sage_include_directories__cython_aliases_from_sage_env_to_sage_setup
37f9b0cbuild/make/Makefile.in: For script packages, make sure that 'sage -f' uninstalls first
456d8a9build/make/Makefile.in (sage_setup-clean): Clean the build tree
f84730esage_setup.clean: Do not clean the sage_setup installation in site-packages
9a6ace2Merge #29847

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Changed commit from 6f1c68e to 9a6ace2

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 8, 2021

Changed dependencies from #29847 to #29847, #32361

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

Changed commit from 9a6ace2 to fb069d4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2021

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

8fccf3ebuild/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): No more DESTDIR staging for Python packages (unless SAGE_SUDO is set)
c7a1171build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Do not use SAGE_SUDO for pip install
a05d57eMerge tag '9.5.beta0' into t/32361/actually_remove_destdir_staging_for_python_packages_to_eliminate_race_conditions_during_python_package_installations
378a034build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Fix for script packages with SAGE_SUDO set
fb069d4Merge #32361

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

comment:17

This doesn't quite work as intended. Weirdly, pip is ignoring the installed packages if it does not also have access to a package index (we use --no-index and also poison proxy variables, so it does not use PyPI).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

Changed commit from fb069d4 to 80f0a9e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

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

80f0a9ebuild/bin/sage-pip-install: Use --find-links=$SAGE_SPKG_WHEELS; on first pass, drop --ignore-installed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

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

ac18343build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Pass options (starting with dashes) on to sage-pip-install
ad18520build/pkgs/pip/spkg-install.in: Use --ignore-installed when installing the wheel

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2021

Changed commit from 80f0a9e to ad18520

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 9, 2021

comment:20

This version works for me. It makes use of the previously built wheels that are kept in $SAGE_VENV/var/lib/sage/wheels/

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2021

Changed commit from ad18520 to 92c855c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2021

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

fb70f8abuild/bin/sage-pip-install: When retrying with --no-deps, also use --ignore-requires-python
92c855cbuild/bin/sage-dist-helpers (sdh_pip_install): Build the wheel with --ignore-requires-python

@mkoeppe

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:23

This seems to work as advertised. Is there room at some point for a pickier build process that actually produces errors when this kind of thing comes up? (Something that could be used by buildbots or developers working new packages but not the default.)

@jhpalmieri
Copy link
Member

comment:24

A related question: how do we convince people to actually check for these warnings?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 17, 2021

comment:25

I think there's room for escalation in a follow-up ticket, but I think it's safer to first bring it to wider testing by developers in the current form.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 17, 2021

comment:26

I've opened #32529 for this.

@jhpalmieri
Copy link
Member

comment:27

I tested this with ipykernel. It does indeed print a warning about a missing dependency, and it's not too hard to see from the log file that it's debugpy.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 17, 2021

comment:28

Thanks for reviewing!

@vbraun
Copy link
Member

vbraun commented Sep 25, 2021

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