Replace OptionalExtension(...., package='bliss') by condition=BlissLibrary().is_present() #25828

timokau · 2018-07-11T15:54:33Z

The last remaining (after #25825) piece of code that uses is_package_installed is OptionalExtension. Since it only provides the package kwarg for convenience, I thought it would make sense to just move to condition and add feature checks to use in module_list.py.

Moreover, OptionalExtension(..., package=....) does not take system packages checked for by spkg-configure.m4 (#27330) into account.

setup.py build_cython is extended by an option --require-features for the benefit of distribution builders who want to ensure a predictable build. For example:

./sage -sh -c "cd src && sage-python -u setup.py build_cython --require-features=sage.features.databases.DatabaseCremona"
....
sage.features.FeatureNotPresentError: Cremona's database of elliptic curves is not available.
'cremona/cremona.db' not found in any of ['/Users/mkoeppe/s/sage/sage-rebasing/local/share']
To install Cremona's database of elliptic curves you can try to run 'sage -i database_cremona_ellcurve'.
Further installation instructions might be available at https://github.com/JohnCremona/ecdata.

On the branch is one module for which a feature test is already available (bliss).

See also:

Move optional sage optimization backends (COIN, CPLEX, Gurobi) to separate Cython packages to remove OptionalExtension problems #28175: Move sage optimization backends to separate Cython packages to remove OptionalExtension problems
Follow-up: src/setup.py: Replace use of is_package_installed_and_updated by sage_conf or features #28815: Don't pass SAGE_PKGS to src/setup.py. Replace OptionalExtension(...., package=....) by condition=Feature.is_present()

CC: @jdemeyer @antonio-rojas @kiwifb @dimpase @embray @isuruf

Component: build

Branch/Commit: u/mkoeppe/optional-extension-features @ b5e205f

Reviewer: Dima Pasechnik

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

The text was updated successfully, but these errors were encountered:

timokau · 2018-07-11T15:54:50Z

comment:1

Jeroen, according to git log you seem most experienced with what is going on in setup.py (second just to William Stein in number of commits). Do you have a opinion?

jdemeyer · 2018-07-11T18:46:18Z

comment:2

I don't know. There is no obvious "best" solution.

timokau · 2018-07-11T19:50:53Z

comment:4

I think #2 (two steps) would probably be the cleaner solution. Creating and then deleting a stub during installation would be pretty hacky.

I tried implementing that but having basically no experience with distutils I'm confused. ext_modules is imported from module_list but then never used. Instead some magic self.distribution.ext_modules that appears out of nowhere is used.

Do you think #1 is feasible?

jdemeyer · 2018-07-11T19:57:27Z

comment:5

Replying to @timokau:

ext_modules is imported from module_list but then never used.

Please read the very last line of src/setup.py

timokau · 2018-07-11T20:13:54Z

comment:6

Oh right, thanks.

So ext_modules is a distutils concept and they expect one single list of modules? If that is the case some pretty ugly hacks would probably be needed to cythonize the extensions in two sets.

mkoeppe · 2019-11-23T19:32:29Z

comment:7

I'd suggest to use #28791 (Implement Feature without using sage.misc.cachefunc, sage.structure.unique_representation) - I have an implementation that does the caching by hand and implements a TrivialUniqueRepresentation in a few lines to remove the dependencies.

mkoeppe · 2019-11-23T19:38:27Z

comment:8

When implementing this ticket, one should watch out to preserve what was intended in #21288.

timokau · 2019-11-24T19:04:36Z

comment:9

I don't currently have time to work on this, so if anyone wants to take over that would be great.

mkoeppe · 2019-11-26T03:31:53Z

Dependencies: #28791

kiwifb · 2019-11-26T22:35:08Z

comment:11

A move away to the current use of is_package_installed is welcome. My only issue with the current implementation example is that it is automagical. The extension is automatically built on detection. Packagers usually prefer enable and then check availability. The default position of sage has always been to build on presence but as a packager I want to be able to have some control mechanism where I can disable the feature even if the necessary bits are present.

This would be a major step in avoid sage's packaging system at build time. Testing time remains an issue as it calls list_packages but nothing short of an overall of the option system in doctest may be able to get rid of it.

mkoeppe · 2019-11-26T22:53:19Z

Changed branch from u/gh-timokau/optional-extension-features to u/mkoeppe/optional-extension-features

mkoeppe · 2019-11-26T23:03:17Z

Changed commit from ecf8693 to e9738ac

mkoeppe · 2019-11-26T23:03:17Z

comment:13

Replying to @kiwifb:

My only issue with the current implementation example is that it is automagical. The extension is automatically built on detection. Packagers usually prefer enable and then check availability. The default position of sage has always been to build on presence but as a packager I want to be able to have some control mechanism where I can disable the feature even if the necessary bits are present.

Great point. We could add options to setup.py build such as --require-feature=sage.features.bliss.BlissLibrary. If this setup.py build is invoked with this option but the feature is not found, an error would be signaled.

New commits:

`5543692`	`Replace is_package_installed("bliss") by features`
`24921a0`	`Implement Feature without using sage.misc.cachefunc, sage.structure.unique_representation`
`e9738ac`	`Merge branch 't/28791/feature_without_using_sage_modules' into t/25828/optional-extension-features`

sagetrac-git · 2019-11-27T00:59:45Z

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

`4392be3`	`./setup.py build_cython --require-features=sage.features.bliss.BlissLibrary`

sagetrac-git · 2019-11-27T00:59:45Z

Changed commit from e9738ac to 4392be3

sagetrac-git · 2019-11-27T01:03:24Z

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

`0575b96`	`WIP: Disable bliss Feature test`

sagetrac-git · 2019-11-27T01:03:24Z

Changed commit from 4392be3 to 0575b96

kiwifb · 2019-11-27T01:05:08Z

comment:16

As a prototype it looks promising but the option is quite complicated. Even when adding proper documentation of the available features. We may want to have some kind of dictionary matching simple option names to features. But that has to come later when we add more stuff.

mkoeppe · 2019-11-27T01:08:13Z

comment:18

I would need help from a distutils expert on how to make the option --require-features also usable with build, rather than just build_cython.

mkoeppe · 2019-11-27T01:11:51Z

comment:19

In the ticket description I have added an example sage.features.databases.DatabaseCremona, which is a StaticFile feature.

Supporting CythonFeature (such as the previously mentioned sage.features.bliss.BlissLibrary) needs more work because it pulls in other modules that are not available at build time:

running build_cython
Enabling Cython debugging support
************************************************************************
Traceback (most recent call last):
  File "setup.py", line 887, in <module>
    ext_modules = ext_modules)
  File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/distutils/dist.py", line 984, in run_command
    cmd_obj.ensure_finalized()
  File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/distutils/cmd.py", line 107, in ensure_finalized
    self.finalize_options()
  File "setup.py", line 296, in finalize_options
    feature.require()
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/features/__init__.py", line 189, in require
    presence = self.is_present()
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/features/__init__.py", line 161, in is_present
    res = self._is_present()
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/features/__init__.py", line 551, in _is_present
    with open(tmp_filename(ext=".pyx"), 'w') as pyx:
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/misc/temporary_file.py", line 149, in tmp_filename
    from sage.misc.misc import SAGE_TMP
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/misc/misc.py", line 51, in <module>
    import sage.misc.prandom as random
  File "/Users/mkoeppe/s/sage/sage-rebasing/src/sage/misc/prandom.py", line 58, in <module>
    from sage.misc.randstate import current_randstate
ModuleNotFoundError: No module named 'sage.misc.randstate'

mkoeppe · 2019-11-27T01:12:45Z

comment:20

Replying to @kiwifb:

As a prototype it looks promising but the option is quite complicated. Even when adding proper documentation of the available features. We may want to have some kind of dictionary matching simple option names to features. But that has to come later when we add more stuff.

Yes, I agree, of course we can make it more user-friendly later.

mkoeppe · 2020-01-13T14:16:40Z

comment:35

Sorry, looks like I lost a number of commits in the rebase.

sagetrac-git · 2020-01-13T15:17:33Z

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

`4392be3`	`./setup.py build_cython --require-features=sage.features.bliss.BlissLibrary`
`ac45c74`	`CythonFeature._is_present: Use version of tmp_filename without dependence on SAGE_TMP`
`8ca2e68`	`src/sage/misc/misc.py: Move import of sage.misc.prandom to functions that need it`
`1cee8f1`	`sage.misc.cython.cython: New options capture_stderr=True, target_dir=None`
`8b85b4d`	`sage.misc.cython: Move global imports into the functions where they are used`
`4a85117`	`sage.features.CythonFeature._is_present: Use tempfile.mkdtemp for cython test file and build`
`fdd4ab7`	`sage.features.CythonFeature._is_present: Improve error reporting in FeatureTestResult`
`4af7b45`	`sage.misc.cython.cython: Replace imported sage_makedirs, restore_cwd by local copies`
`aa87fab`	`Merge tag '9.0.beta10' into t/25828/optional-extension-features`
`ef1219e`	`Merge tag '9.1.beta0' into HEAD`

sagetrac-git · 2020-01-13T15:17:33Z

Changed commit from 2c71083 to ef1219e

dimpase · 2020-01-14T09:32:36Z

comment:37

is it ready now?

dimpase · 2020-01-14T17:06:14Z

comment:39

something does not work with the build (docs get built, but then sage won't start)
This is running make ptest (without bliss installed, I think)

...
[dochtml] [thematic_] ... done (4882 js index entries)
[dochtml] [thematic_] The HTML pages are in local/share/doc/sage/html/en/thematic_tutorials.
[dochtml] Build finished. The built documents can be found in /home/scratch2/dimpase/sage/sage/local/share/doc/sage/html/en/thematic_tutorials
[dochtml] Elapsed time: 486.8 seconds.
[dochtml] Done building the documentation!
make[3]: Leaving directory '/home/scratch2/dimpase/sage/sage/build/make'
make -j12 '/home/scratch2/dimpase/sage/sage/local/etc/sage-started.txt'
make[3]: Entering directory '/home/scratch2/dimpase/sage/sage/build/make'
make[3]: warning: -jN forced in submake: disabling jobserver mode.
if [ -z "$SAGE_INSTALL_FETCH_ONLY" ]; then \
        cd /home/scratch2/dimpase/sage/sage/src && source bin/sage-env && \
        sage-logger -p 'time make -j12 sage' '/home/scratch2/dimpase/sage/sage/logs/pkgs/sagelib-9.1.beta0.log'; \
fi
[sagelib-9.1.beta0] make[4]: Entering directory '/home/scratch2/dimpase/sage/sage/src'
[sagelib-9.1.beta0] make[4]: warning: -jN forced in submake: disabling jobserver mode.
[sagelib-9.1.beta0] cd . && export                                    \
[sagelib-9.1.beta0]     SAGE_ROOT=/doesnotexist                               \
[sagelib-9.1.beta0]     SAGE_SRC=/doesnotexist                                \
[sagelib-9.1.beta0]     SAGE_SRC_ROOT=/doesnotexist                           \
[sagelib-9.1.beta0]     SAGE_DOC_SRC=/doesnotexist                            \
[sagelib-9.1.beta0]     SAGE_BUILD_DIR=/doesnotexist                          \
[sagelib-9.1.beta0]     SAGE_PKGS=/home/scratch2/dimpase/sage/sage/build/pkgs                \
[sagelib-9.1.beta0] && sage-python -u setup.py --no-user-cfg build install
[sagelib-9.1.beta0] /home/scratch2/dimpase/sage/sage/src/bin/sage-env: line 130: cd: /doesnotexist: No such file or directory
[sagelib-9.1.beta0] Warning: overwriting SAGE_ROOT environment variable:
[sagelib-9.1.beta0] Old SAGE_ROOT=/doesnotexist
[sagelib-9.1.beta0] New SAGE_ROOT=
[sagelib-9.1.beta0] /tmp/tmp_sage_cython_featuref0w8sij7/target/featuretest.cpp:652:10: fatal error: bliss/graph.hh: No such file or directory
[sagelib-9.1.beta0]  #include "bliss/graph.hh"
[sagelib-9.1.beta0]           ^~~~~~~~~~~~~~~~
[sagelib-9.1.beta0] compilation terminated.
[sagelib-9.1.beta0] Discovering Python/Cython source code....
[sagelib-9.1.beta0] Discovered Python/Cython sources, time: 0.01 seconds.
[sagelib-9.1.beta0] running build
[sagelib-9.1.beta0] Generating auto-generated sources
[sagelib-9.1.beta0] Building interpreters for fast_callable
[sagelib-9.1.beta0] running build_cython
[sagelib-9.1.beta0] Enabling Cython debugging support
[sagelib-9.1.beta0] Updating Cython code....
[sagelib-9.1.beta0] Finished Cythonizing, time: 1.49 seconds.
[sagelib-9.1.beta0] running build_py
[sagelib-9.1.beta0] running build_ext
[sagelib-9.1.beta0] Executing 0 commands (using 1 thread)
[sagelib-9.1.beta0] Time to execute 0 commands: 0.11 seconds.
[sagelib-9.1.beta0] Total time spent compiling C/C++ extensions: 0.14 seconds.
[sagelib-9.1.beta0] running install
[sagelib-9.1.beta0] running install_lib
[sagelib-9.1.beta0] running install_egg_info
[sagelib-9.1.beta0] Removing /home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages/sage-9.1.beta0-py3.7.egg-info
[sagelib-9.1.beta0] Writing /home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages/sage-9.1.beta0-py3.7.egg-info
[sagelib-9.1.beta0] Cleaning up stale installed files....
[sagelib-9.1.beta0] - cleaning build/lib.linux-x86_64-3.7
[sagelib-9.1.beta0] - cleaning /home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages
[sagelib-9.1.beta0] Finished cleaning, time: 0.15 seconds.
[sagelib-9.1.beta0] if [ "$UNAME" = "CYGWIN" ]; then                         \
[sagelib-9.1.beta0]     sage-rebase.sh "$SAGE_LOCAL" 2>/dev/null;            \
[sagelib-9.1.beta0] fi
[sagelib-9.1.beta0] make[4]: Leaving directory '/home/scratch2/dimpase/sage/sage/src'
[sagelib-9.1.beta0]
[sagelib-9.1.beta0] real        0m3.029s
[sagelib-9.1.beta0] user        0m2.510s
[sagelib-9.1.beta0] sys 0m0.419s
"/home/scratch2/dimpase/sage/sage/local/bin/sage-starts"

Testing that Sage starts...
[2020-01-14 15:30:54] SageMath version 9.1.beta0, Release Date: 2020-01-10
Yes, Sage starts.
make[3]: Leaving directory '/home/scratch2/dimpase/sage/sage/build/make'
make[2]: Leaving directory '/home/scratch2/dimpase/sage/sage/build/make'

real    46m41.281s
user    276m31.335s
sys     13m36.772s
Sage build/upgrade complete!
make[1]: Leaving directory '/home/scratch2/dimpase/sage/sage'
./sage -t -p --all --logfile=logs/ptest.log
Traceback (most recent call last):
  File "/home/scratch2/dimpase/sage/sage/src/bin/sage-runtests", line 177, in <module>
    from sage.doctest.control import DocTestController
  File "/home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages/sage/doctest/control.py", line 33, in <module>
    from .sources import FileDocTestSource, DictAsObject
  File "/home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages/sage/doctest/sources.py", line 33, in <module>
    from .parsing import SageDocTestParser
  File "/home/scratch2/dimpase/sage/sage/local/lib/python3.7/site-packages/sage/doctest/parsing.py", line 66, in <module>
    from sage.all import RealIntervalField
ImportError: cannot import name 'RealIntervalField' from 'sage.all' (unknown location)
make: *** [Makefile:172: ptest] Error 1

I guess

[sagelib-9.1.beta0] /tmp/tmp_sage_cython_featuref0w8sij7/target/featuretest.cpp:652:10: fatal error: bliss/graph.hh: No such file or directory

points somewhere...

mkoeppe · 2020-01-14T20:35:16Z

comment:40

Thanks for testing! I'll investigate

mkoeppe · 2020-01-16T03:17:10Z

comment:41

Replying to @dimpase:

something does not work with the build

ImportError: cannot import name 'RealIntervalField' from 'sage.all' (unknown location)
make: *** [Makefile:172: ptest] Error 1

Yes, I also end up with a broken build. I'll look into it more.

I guess

[sagelib-9.1.beta0] /tmp/tmp_sage_cython_featuref0w8sij7/target/featuretest.cpp:652:10: fatal error: bliss/graph.hh: No such file or directory

points somewhere...

No, that is just the output of the feature test. I will try to make it look less scary.

$ ./sage -c 'from sage.features.bliss import BlissLibrary; print(BlissLibrary().is_present())'
/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/tmp_sage_cython_featurempyw50ji/target/featuretest.cpp:653:10: fatal error: 'bliss/graph.hh' file not found
#include "bliss/graph.hh"
         ^~~~~~~~~~~~~~~~
1 error generated.
FeatureTestResult('Bliss', False)

sagetrac-git · 2020-04-14T01:47:02Z

Changed commit from ef1219e to b5e205f

sagetrac-git · 2020-04-14T01:47:02Z

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

`b5e205f`	`WIP: Add progress output`

mkoeppe · 2020-10-30T23:15:09Z

Changed author from Matthias Koeppe, Timo Kaufmann to none

mkoeppe · 2020-10-30T23:15:09Z

comment:46

OptionalExtension is no longer used. Let's close this ticket.

mkoeppe · 2020-10-30T23:15:09Z

Changed reviewer from Dima Pasechnik to none

dimpase · 2020-10-31T20:19:19Z

Reviewer: Dima Pasechnik

timokau added this to the sage-8.3 milestone Jul 11, 2018

timokau added c: build labels Jul 11, 2018

mkoeppe modified the milestones: sage-8.3, sage-9.0 Nov 23, 2019

This comment has been minimized.

Sign in to view

mkoeppe changed the title ~~Move OptionalExtension to features~~ Move OptionalExtension to features, don't pass SAGE_PKGS to src/setup.py Nov 26, 2019

This comment has been minimized.

Sign in to view

dimpase added s: needs work and removed s: needs review labels Jan 13, 2020

mkoeppe added s: needs review and removed s: needs work labels Jan 14, 2020

mkoeppe added s: needs work and removed s: needs review labels Jan 19, 2020

mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 9, 2020

mkoeppe removed this from the sage-9.2 milestone Jul 2, 2020

mkoeppe added the wishlist item label Jul 2, 2020

mkoeppe added s: needs review and removed wishlist item labels Oct 30, 2020

dimpase added s: positive review and removed s: needs review labels Oct 31, 2020

fchapoton removed the s: positive review label Nov 8, 2020

fchapoton closed this as completed Nov 8, 2020

Replace OptionalExtension(...., package='bliss') by condition=BlissLibrary().is_present() #25828

Replace OptionalExtension(...., package='bliss') by condition=BlissLibrary().is_present() #25828

Comments

timokau commented Jul 11, 2018

timokau commented Jul 11, 2018

jdemeyer commented Jul 11, 2018

timokau commented Jul 11, 2018

jdemeyer commented Jul 11, 2018

timokau commented Jul 11, 2018

mkoeppe commented Nov 23, 2019

mkoeppe commented Nov 23, 2019

timokau commented Nov 24, 2019

This comment has been minimized.

mkoeppe commented Nov 26, 2019

kiwifb commented Nov 26, 2019

mkoeppe commented Nov 26, 2019

mkoeppe commented Nov 26, 2019

mkoeppe commented Nov 26, 2019

sagetrac-git mannequin commented Nov 27, 2019

sagetrac-git mannequin commented Nov 27, 2019

sagetrac-git mannequin commented Nov 27, 2019

sagetrac-git mannequin commented Nov 27, 2019

kiwifb commented Nov 27, 2019

This comment has been minimized.

mkoeppe commented Nov 27, 2019

mkoeppe commented Nov 27, 2019

mkoeppe commented Nov 27, 2019

mkoeppe commented Jan 13, 2020

sagetrac-git mannequin commented Jan 13, 2020

sagetrac-git mannequin commented Jan 13, 2020

dimpase commented Jan 14, 2020

dimpase commented Jan 14, 2020

mkoeppe commented Jan 14, 2020

mkoeppe commented Jan 16, 2020

sagetrac-git mannequin commented Apr 14, 2020

sagetrac-git mannequin commented Apr 14, 2020

mkoeppe commented Oct 30, 2020

mkoeppe commented Oct 30, 2020

mkoeppe commented Oct 30, 2020

dimpase commented Oct 31, 2020