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

Create PEP 503 simple repository for wheels built during installation #30527

Closed
mkoeppe opened this issue Sep 8, 2020 · 49 comments
Closed

Create PEP 503 simple repository for wheels built during installation #30527

mkoeppe opened this issue Sep 8, 2020 · 49 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Sep 8, 2020

As a follow-up to #29500 (Install all Python packages via pip wheel, store wheels in $SAGE_LOCAL/var/lib/sage/wheels):

We create a PEP 503 compliant Simple Repository index in $SAGE_SPKG_WHEELS. Then users can create their virtual environment using tools such as pipenv [[source]] or pip install --index-url.

We do this using the command dir2pi from https://pypi.org/project/pip2pi/

Follow-up:


References:

CC: @slel @tobiasdiez @embray @jhpalmieri

Component: build

Keywords: sd111

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/pep-503-simple-repository-for-wheels @ 333e50c

Reviewer: Tobias Diez

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Sep 8, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

comment:1

piprepo can be used for this task, but it has excessive dependencies

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

Dependencies: #29500

@tobiasdiez
Copy link
Contributor

comment:3

There is also pypiserver, see eg https://stackoverflow.com/a/51528588/873661 for how to use it with pipenv

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

comment:4

Well, we don't want to run a server for this, but following this link I found https://github.com/wolever/pip2pi

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:7

Yeah I agree, a server is not required (although the overhead of creating a simple server on listing on localhost should be pretty minimal). If pip2pi works with pipenv and pip, then that's perfect!

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

New commits:

8aa6fd9build/bin/sage-dist-helpers (sdh_pip_install): Build a wheel, store it
d369aabbuild/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): New, factored out from sdh_pip_install
2d435abbuild/pkgs/numpy/spkg-install.in: Install via setup.py bdist_wheel
55993b6build/bin/sage-dist-helpers: Fixup
0a64674build/pkgs/gambit/spkg-install.in: Install via bdist_wheel
ca58693build/pkgs/pillow/spkg-install.in: Install via bdist_wheel
207d80fbuild/pkgs/pip2pi: New
2555e19build/make/install: At the end, update the repository index

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 8, 2020

Commit: 2555e19

@tobiasdiez
Copy link
Contributor

comment:10

What is the reason that sage needs to build wheels for standard packages, and not just downloads them from pypi?

Is it because some packages have patches applied? In this case, I would propose to migrate these patches to proper forks of the packages which are then published to pypi including their wheel. This has a few advantages, mainly that the build of sage is speedup and simplified, and that the wider python community can profit from these patches.

What do you think?

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:11

Replying to @tobiasdiez:

What is the reason that sage needs to build wheels for standard packages, and not just downloads them from pypi?

Sage-the-distribution is a deployment mechanism. Compiling all packages from source gives us full control and eliminates possible ABI problems from mixing prebuilt wheels that were compiled on different systems. Building from source also has advantages such as generating architecture-specific code (see #27122).

Is it because some packages have patches applied?

In some cases, yes, in other cases, just specific configuration.

In this case, I would propose to migrate these patches to proper forks of the packages which are then published to pypi including their wheel. This has a few advantages, mainly that the build of sage is speedup and simplified, and that the wider python community can profit from these patches.

We don't have intentions to take over maintenance of these packages. Whenever possible, we send our patches upstream. In many cases, it is just a matter of mismatched release schedules, and we have to keep patches only for a limited time until upstream makes a new stable release including a particular patch. For example, consider the numpy patch added in #29766. It is actually from upstream, but not included in any stable release. But we need the patch because we want Sage-the-distribution to support a particular platform.

@tobiasdiez
Copy link
Contributor

comment:12

Thanks for the detailed explanation. Makes things clearer for me now.

Compiling all packages from source gives us full control and eliminates possible ABI problems from mixing prebuilt wheels that were compiled on different systems. Building from source also has advantages such as generating architecture-specific code

But isn't the concept of compile-once-reuse-often the main point of wheels? By design, it's the responsibility of the the maintainer (= publisher of the wheel) that the wheel is useable and optimized for the particular platforms they are published. This also make sense since she is the export of the code of the library.

I feel none of the advantages of wheels outlined in https://www.python.org/dev/peps/pep-0427/ can really prove effective if wheels are rebuilt from code every time.

The wheel binary package format frees installers from having to know about the build system, saves time by amortizing compile time over many installations, and removes the need to install a build system in the target environment.

In my opinion, the build process of sage would immensely profit from all these advantages.

Anyway, this discussion is somewhat orthogonal to this ticket (sorry!). So concerning the changes, the code looks good to me. I'm only wondering if the pip2 support in the uninstallation script is really necessary (since only python3 is supported now). However, I don't feel familiar enough with the build script to change the status to positive review.

@tobiasdiez
Copy link
Contributor

Reviewer: Tobias Diez,

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:13

Replying to @tobiasdiez:

But isn't the concept of compile-once-reuse-often the main point of wheels? By design, it's the responsibility of the the maintainer (= publisher of the wheel) that the wheel is useable and optimized for the particular platforms they are published. This also make sense since she is the export of the code of the library.

In practice, wheels are offered on PyPI with the broadest possible platform compatibility, typically using https://github.com/pypa/manylinux -- this is the opposite of "optimized".

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:14

Replying to @tobiasdiez:

I'm only wondering if the pip2 support in the uninstallation script is really necessary (since only python3 is supported now).

Yes, this should be removed, but I prefer to keep tickets narrowly focused. A separate ticket can do this.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:15

Replying to @tobiasdiez:

But isn't the concept of compile-once-reuse-often the main point of wheels?

Wheels are a format for deployment. Public distribution of wheels on a package index is one use case. Here on this ticket we use it for local deployment -- users can installs these wheels into their venvs.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:16

Replying to @tobiasdiez:

I feel none of the advantages of wheels outlined in https://www.python.org/dev/peps/pep-0427/ can really prove effective if wheels are rebuilt from code every time. [...]
In my opinion, the build process of sage would immensely profit from all these advantages.

I prefer to think about this not as a question of improving "the" build process of sage, but to offer more options for building sage. This is for example also the purpose of the modularization plan outlined in #29705 - make modularized parts of the Sage library deployable via PyPI.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

Changed reviewer from Tobias Diez, to Tobias Diez, ...

@tobiasdiez
Copy link
Contributor

comment:18

Replying to @mkoeppe:

Replying to @tobiasdiez:

I'm only wondering if the pip2 support in the uninstallation script is really necessary (since only python3 is supported now).

Yes, this should be removed, but I prefer to keep tickets narrowly focused. A separate ticket can do this.

It's added in the new file sage-pip-uninstall.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 9, 2020

comment:19

Ok, I'll clean it up - but in #29500 (dependency of this ticket)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2020

Changed commit from 2555e19 to e66243f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2020

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

5a747c4build/bin/sage-pip-{install,uninstall}: Remove pip2 support
e66243fMerge branch 't/29500/install_all_python_packages_via_pip_wheel__create_pep_503_simple_repository_for_wheels' into t/30527/pep-503-simple-repository-for-wheels

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 5, 2020

comment:29

Replying to @vbraun:

Looks like some script is silently ignoring errors (and there is an error, too):

$ ./sage -i pip2pi
[...]
[sagelib-9.3.beta2] Finished cleaning, time: 0.15 seconds.
[sagelib-9.3.beta2] 
[sagelib-9.3.beta2] real	0m2.307s
[sagelib-9.3.beta2] user	0m1.765s
[sagelib-9.3.beta2] sys	0m0.539s
touch "/home/release/Sage/local/var/lib/sage/installed/sagelib-9.3.beta2"
make[1]: Leaving directory '/home/release/Sage/build/make'

real	0m4.280s
user	0m3.292s
sys	0m0.942s
Traceback (most recent call last):
  File "/home/release/Sage/local/bin/dir2pi", line 8, in <module>
    sys.exit(dir2pi())
  File "/home/release/Sage/local/lib64/python3.8/site-packages/libpip2pi/commands.py", line 308, in dir2pi
    return _dir2pi(option, argv)
  File "/home/release/Sage/local/lib64/python3.8/site-packages/libpip2pi/commands.py", line 377, in _dir2pi
    cgi.escape(pkg_dir_name),
AttributeError: module 'cgi' has no attribute 'escape'
Sage build/upgrade complete!

Looks like this package is not ready for Python 3.8, wolever/pip2pi#111

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 5, 2020

comment:30

wolever/pip2pi#106

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2020

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

ee4a743Merge tag '9.3.beta2' into t/30527/pep-503-simple-repository-for-wheels
9bae6a1build/pkgs/pip2pi: Make it a normal package
4e7f968build/pkgs/pip2pi/patches: Add patch for python >=3.8 support

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2020

Changed commit from e79de96 to 4e7f968

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2020

Changed commit from 4e7f968 to 333e50c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 6, 2020

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

333e50cbuild/make/install: Do not ignore errors from dir2pi

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2020

comment:34

Replying to @vbraun:

Also docbuild fails on buildbots but not on my machine with

[dochtml] [spkg     ] /Users/buildbot-sage/slave/sage_git/build/src/doc/en/reference/spkg/pip2pi.rst: WARNING: document isn't included in any toctree

To debug this on the affected buildbots, it should be checked whether the file index.rst in the same directory is generated correctly by bootstrap.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 22, 2020

comment:36

Waiting for review

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2021

comment:37

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 15, 2021
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2021

comment:38

Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@jhpalmieri
Copy link
Member

comment:39

I tried make pip2pi (after rebasing to the latest beta), and although the package built, I got an error message afterwards:

[pip2pi-0.8.1] Finished installing pip2pi-0.8.1

real	0m33.466s
user	0m3.670s
sys	0m1.412s
Traceback (most recent call last):
  File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage-9.5.beta2/local/bin/dir2pi", line 8, in <module>
    sys.exit(dir2pi())
  File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage-9.5.beta2/local/lib/python3.9/site-packages/libpip2pi/commands.py", line 312, in dir2pi
    return _dir2pi(option, argv)
  File "/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage-9.5.beta2/local/lib/python3.9/site-packages/libpip2pi/commands.py", line 340, in _dir2pi
    raise ValueError("no such directory: %r" %(pkgdir, ))
ValueError: no such directory: ''
Error updating PEP 503 simple repository index
make: *** [pip2pi] Error 1

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 30, 2021

comment:40

Thanks for testing. Looks like this now needs updating to the recent build system changes (probably the SAGE_LOCAL/SAGE_VENV split). There's no urgency to this ticket because I learned that one can use --find-links instead of --index-url. So I've changed the milestone

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 3, 2023

Closing as unnecessary

@mkoeppe mkoeppe closed this as not planned Won't fix, can't repro, duplicate, stale Nov 3, 2023
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

4 participants