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

New option "./configure --enable-wheels" #32874

Closed
mkoeppe opened this issue Nov 15, 2021 · 104 comments
Closed

New option "./configure --enable-wheels" #32874

mkoeppe opened this issue Nov 15, 2021 · 104 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 15, 2021

When ./configure --disable-editable is in use (the default before #32406), the Sage library is installed using its custom incremental implementation of setup.py install -- which installs its files in site-packages and then removes the files that it does not know about (the "cleaner").

Problem 1: The cleaner is incompatible with namespace packages and trying to fix it would lead to complicated and awkward code (#32927). (Besides, setup.py install is deprecated since https://setuptools.pypa.io/en/latest/history.html#v58-3-0)

Problem 2: Because of the direct installation, we do not have wheels for sagemath-standard available (even if make wheels is used explicitly). Such wheels can be useful for making separate venvs.

In this ticket:

  • We create a new option ./configure --enable-wheels (which can be used both with ./configure --enable-editable and ./configure --disable-editable).
  • If enabled, installation of sagelib will go through wheel building and installation - like we do with all other Python packages. This enables modularized builds (Refactor sagemath-standard through sagemath-{categories,environment,repl} #34587).
  • If disabled, there is no change.

A trivial ./sage -b is much slower with ./configure --disable-editable --enable-wheels compared to ./configure --disable-editable (from 28s as per timings in #32406 comment:21 to 70s).
This ticket brings a number of speed improvements to mitigate this.

CC: @jhpalmieri

Component: build

Author: Matthias Koeppe

Branch: 25898ba

Reviewer: Kwankyu Lee

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

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

mkoeppe commented Nov 15, 2021

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2021

New commits:

b9b1dbaEnable editable mode also for other sage packages
a257621Add error handling
89bc3eeMerge branch 'develop' of git://github.com/sagemath/sage into public/build/inplace_ext
7bd6ce4Partly revert "Enable editable mode also for other sage packages"
3ef9c1dMerge #32713
50dd26cbuild/pkgs/sagelib/spkg-install: Use helper function sdh_pip_editable_install
b5c40a8build/pkgs/sagelib/spkg-install: In non-editable mode, Use sdh_pip_install instead of 'setup.py install'

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2021

Commit: b5c40a8

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Nov 15, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2022

Changed dependencies from #32713 to #32406

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

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

8619b29configure.ac: Make --enable-editable the default
6513b6bREADME.md: Explain configure --disable-editable
ff8710edocker/Dockerfile: Use configure --disable-editable
7221a76src/setup.py: Do not run find_namespace_packages for 'setup.py dist_info'
a911e0fsrc/MANIFEST.in: prune sage_docbuild, doc
edfa3b4build/pkgs/sagelib/spkg-install: Use helper function sdh_pip_editable_install
f7ea602build/pkgs/sagelib/spkg-install: In non-editable mode, Use sdh_pip_install instead of 'setup.py install'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

Changed commit from b5c40a8 to f7ea602

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Remove use of "setup.py install" for sagelib Remove use of "setup.py install" for sagelib when ./configure --disable-editable is in use Jul 22, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 22, 2022

comment:10
[sagelib-9.7.beta5] sage-flock: error: argument LOCK: can't create '/doesnotexist/var/lock': [Errno 30] Read-only file system: '/doesnotexist'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

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

df1e7ebbuild/bin/sage-pip-install: Put lock file in SAGE_VENV, not SAGE_LOCAL
f358962build/pkgs/sagelib/spkg-install: No need to set env var PYTHON
c761788build/bin/sage-pip-uninstall: Put lock file in SAGE_VENV, not SAGE_LOCAL
99d4129build/pkgs/sagelib/spkg-install [SAGE_EDITABLE=no]: Use --no-build-isolation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 22, 2022

Changed commit from f7ea602 to 99d4129

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Jul 23, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2022

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

07f5cdebuild/pkgs/sagelib/spkg-install: Use helper function sdh_pip_editable_install
3cdf86ebuild/pkgs/sagelib/spkg-install: In non-editable mode, Use sdh_pip_install instead of 'setup.py install'
459ee2fbuild/bin/sage-pip-install: Put lock file in SAGE_VENV, not SAGE_LOCAL
3de2608build/pkgs/sagelib/spkg-install: No need to set env var PYTHON
e6eae68build/bin/sage-pip-uninstall: Put lock file in SAGE_VENV, not SAGE_LOCAL
e829b08build/pkgs/sagelib/spkg-install [SAGE_EDITABLE=no]: Use --no-build-isolation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 26, 2022

Changed commit from 99d4129 to e829b08

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 3, 2022

comment:64

Replying to Kwankyu Lee:

./configure --enable-wheels doesn't seem to work for me, even after ./bootstrap (is this necessary?)

sage -b after ./configure --disable-editable does not install wheel. sage -b just takes 20 seconds.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 3, 2022

comment:65

Yes, there are 4 modes of operation now, all combinations of --{en,dis}able-editable and --{en,dis}able-wheels should work

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 4, 2022

comment:66

What doesn't work is to configure those options separately: ./configure --enable-wheels and then ./configure --disable-editable. I guess this is normal.

Yes, ./configure --disable-editable --enable-wheels does build the wheel :)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:67

To add options to the previous list of configure options, there's unfortunately no shorter idiom than

eval ./configure $(./config.status --config) NEW-OPTIONS...

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 4, 2022

comment:68

Making --enable-wheels default eventually is necessary for sagelib modularization. Right?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:69

I'd say it will be necessary as soon as the first external packages arrive that declare a dependency on a modularized distribution rather than on sagemath-standard.

Until then we can get away with keeping both --enable-editable and --disable-editable --disable-wheels monolithic; and we can use --disable-editable --enable-wheels for working on the details of the modularized distributions.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:70

(see ticket description of #34587)

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 4, 2022

comment:71

Questions on changes in src/MANIFEST.in:

  1. Is this line include MANIFEST.in necessary? Perhaps because src/MANIFEST.in is a symlink?

  2. How was this list made ? Could the hard-coded list be still valid in future?

include sage/modular/arithgroup/farey_symbol.h
include sage/cpython/debugimpl.c
include sage/graphs/base/boost_interface.cpp
include sage/graphs/cliquer/cl.c
include sage/graphs/graph_decompositions/sage_tdlib.cpp
include sage/libs/eclib/wrap.cpp
include sage/libs/linkages/padics/relaxed/flint_helper.c
include sage/misc/inherit_comparison_impl.c
include sage/modular/arithgroup/farey.cpp
include sage/modular/arithgroup/sl2z.cpp
include sage/rings/bernmm/bern_modp.cpp
include sage/rings/bernmm/bern_modp_util.cpp
include sage/rings/bernmm/bern_rat.cpp
include sage/rings/bernmm/bernmm-test.cpp
include sage/rings/padics/transcendantal.c
include sage/rings/polynomial/weil/power_sums.c
include sage/schemes/hyperelliptic_curves/hypellfrob/hypellfrob.cpp
include sage/schemes/hyperelliptic_curves/hypellfrob/recurrences_ntl.cpp
include sage/schemes/hyperelliptic_curves/hypellfrob/recurrences_zn_poly.cpp
include sage/stats/distributions/dgs_bern.c
include sage/stats/distributions/dgs_gauss_dp.c
include sage/stats/distributions/dgs_gauss_mp.c
include sage/symbolic/ginac/*.cpp   

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 4, 2022

Reviewer: Kwankyu Lee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

Changed commit from 11507a5 to 974c7fd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

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

974c7fdsrc/MANIFEST.in: Remove redundant entries for MANIFEST.in, pyproject.toml

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:74

Replying to Kwankyu Lee:

Questions on changes in src/MANIFEST.in:

  1. Is this line include MANIFEST.in necessary? Perhaps because src/MANIFEST.in is a symlink?

No, not necessary any more; it probably was necessary with prehistoric versions of setuptools.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

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

25898basrc/MANIFEST.in: Exclude generated file farey_symbol.h; add comments

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2022

Changed commit from 974c7fd to 25898ba

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:76

Replying to Kwankyu Lee:

  1. How was this list made ?

I've added a comment (and fixed a mistake that I had made along the way).

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 4, 2022

comment:77

Thanks.

It works well and looks good to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2022

comment:78

Thank you!

@vbraun
Copy link
Member

vbraun commented Oct 16, 2022

@antonio-rojas
Copy link
Contributor

Changed commit from 25898ba to none

@antonio-rojas
Copy link
Contributor

comment:80

This makes tests depend on sage_setup, so one gets failures when testing an installed sage

sage -t --long --random-seed=334405888414880658562662015982432628863 /usr/lib/python3.10/site-packages/sage/misc/package_dir.py
**********************************************************************
File "/usr/lib/python3.10/site-packages/sage/misc/package_dir.py", line 108, in sage.misc.package_dir.read_distribution
Failed example:
    from sage_setup.find import read_distribution
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.package_dir.read_distribution[1]>", line 1, in <module>
        from sage_setup.find import read_distribution
    ModuleNotFoundError: No module named 'sage_setup'
**********************************************************************
File "/usr/lib/python3.10/site-packages/sage/misc/package_dir.py", line 109, in sage.misc.package_dir.read_distribution
Failed example:
    read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'tdlib.pyx'))
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.package_dir.read_distribution[2]>", line 1, in <module>
        read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'tdlib.pyx'))
    NameError: name 'read_distribution' is not defined
**********************************************************************
File "/usr/lib/python3.10/site-packages/sage/misc/package_dir.py", line 111, in sage.misc.package_dir.read_distribution
Failed example:
    read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'modular_decomposition.py'))
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.package_dir.read_distribution[3]>", line 1, in <module>
        read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'modular_decomposition.py'))
    NameError: name 'read_distribution' is not defined
**********************************************************************

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2022

comment:81

I've opened #34674 for this

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 17, 2022

comment:82

A question to educate myself: In what environment, sage_setup module is not available? Is this a linux package?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2022

comment:83

In distribution packaging (Antonio does the packaging for Arch Linux), sage_setup is typically not available at runtime.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 17, 2022

comment:84

Okay. Thanks.

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