Don't use is_package_installed('bliss')

[mantepse]

In 6.9.beta5 I have:

sage: p = posets.IntegerPartitions(20)
sage: timeit("p.canonical_label()")
5 loops, best of 3: 180 ms per loop

whereas in 7.4.beta1

sage: timeit("p.canonical_label()")
5 loops, best of 3: 1.58 s per loop

CC: @jm58660 @fchapoton @jdemeyer @mkoeppe @tscrim

Component: graph theory

Author: Frédéric Chapoton

Branch/Commit: 253517d

Reviewer: François Bissey, Jeroen Demeyer

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

[jm58660]

comment:1

Can you check canonical label for plain graphs? Are you sure that you don't have Bliss installed in 6.9 but not in 7.4?

[mantepse]

comment:2

(no, i haven't installed bliss in either version)

The problem is actually elsewhere:

sage: timeit('p._hasse_diagram.canonical_label(algorithm="sage")')
5 loops, best of 3: 93 ms per loop

In 7.4, all the time is eaten by

sage: timeit("is_package_installed('bliss')")
5 loops, best of 3: 1.37 s per loop

I think that Posets.canonical_label should also accept the optional argument algorithm.

[jm58660]

comment:3

Replying to @mantepse:

In 7.4, all the time is eaten by

sage: timeit("is_package_installed('bliss')")
5 loops, best of 3: 1.37 s per loop

Is the problem installed_packages()? On my system it seems to be the problem.

I think that Posets.canonical_label should also accept the optional argument algorithm.

True. I can add it, but let's first check what is wrong here.

[jm58660]

comment:4

I think I got it. It does

proc = subprocess.Popen(["pip", "list"], stdout=subprocess.PIPE)
stdout = proc.communicate()[0]
return dict((name.lower(), version) for name,version in PIP_VERSION.findall(stdout))

and this is clearly too much for every function call.

[jm58660]

comment:5

Jeroen is propably interested in this.

[jm58660]

comment:6

As the problem is now in the ticket #21291 and the keyword is added at #21293 we can close this one.

[mantepse]

Reviewer: Martin Rubey

[jdemeyer]

comment:9

Replying to @mantepse:

is_package_installed('bliss')

Don't do that. Use the Easier to ask for forgiveness than permission principle: assume that Bliss is installed and fail gracefully if not.

[mantepse]

comment:10

So all of these (except the ones in package) are wrong:

-*- mode: grep; default-directory: "~/sage-develop/src/sage/" -*-
Grep started at Fri Aug 19 13:31:58

grep -r -nH -e "is_package_installed(" *
databases/cremona.py:827:            elif is_package_installed('database_cremona_ellcurve'):
databases/cremona.py:1676:        if is_package_installed('database_cremona_ellcurve'):
game_theory/normal_form_game.py:1320:            if is_package_installed('lrslib'):
game_theory/normal_form_game.py:1326:            if not is_package_installed('lrslib'):
geometry/polyhedron/base.py:3694:        if not is_package_installed('lrslib'):
graphs/generic_graph.py:7877:            elif is_package_installed("python_igraph"):
graphs/generic_graph.py:20258:             is_package_installed('bliss'))):
graphs/generic_graph.py:20927:             is_package_installed('bliss') and
graphs/graph_generators.py:1199:        if not is_package_installed("buckygen"):
graphs/graph_generators.py:1284:        if not is_package_installed("benzene"):
graphs/graph_generators.py:1437:        if not is_package_installed("plantri"):
graphs/graph_generators.py:1636:        if not is_package_installed("plantri"):
graphs/graph_generators.py:1790:        if not is_package_installed("plantri"):
graphs/lovasz_theta.py:70:    if not is_package_installed('csdp'):
graphs/generators/classical_geometries.py:1292:    if not is_package_installed('gap_packages'):
groups/perm_gps/permgroup.py:193:        if not is_package_installed('gap_packages'):
groups/perm_gps/permgroup.py:1686:            if not is_package_installed('database_gap'):
groups/perm_gps/permgroup.py:1739:            if not is_package_installed('database_gap'):
groups/perm_gps/permgroup.py:4117:        if not is_package_installed('gap_packages'):
groups/generic.py:1407:        if not is_package_installed('database_gap'):
misc/package.py:294:def is_package_installed(package):
misc/package.py:300:        sage: is_package_installed('pari')
misc/package.py:305:        sage: is_package_installed('matplotli')
Übereinstimmungen in Binärdatei misc/package.pyc.
rings/polynomial/multi_polynomial_sequence.py:1432:                if not is_package_installed('fes'):

Grep finished (matches found) at Fri Aug 19 13:31:58

[jm58660]

comment:11

Why can't Sage just check installed packages on startup and make a list to some global variable?

[jdemeyer]

comment:12

Replying to @jm58660:

Why can't Sage just check installed packages on startup and make a list to some global variable?

Because that would be very slow, see #21291.

[mantepse]

comment:13

Some are possibly OK, for example in cremona.py, because they only determine the error message.

However, that's not quite true either: if I'm going to catch an error, I possibly rely on it being thrown quickly...

[jdemeyer]

comment:14

Replying to @mantepse:

So all of these (except the ones in package) are wrong:

At least the ones which involve an OptionalExtension (like bliss) should be replaced by just importing the module.

[jm58660]

comment:15

Replying to @jdemeyer:

Replying to @jm58660:

Why can't Sage just check installed packages on startup and make a list to some global variable?

Because that would be very slow, see #21291.

Too slow to do it once at startup?

[mantepse]

comment:16

Replying to @jdemeyer:

Replying to @jm58660:

Why can't Sage just check installed packages on startup and make a list to some global variable?

Because that would be very slow, see #21291.

I do not understand this answer. Compiling the list once does not take very long. The problem occurs only because sage is doing it over and over again.

[mantepse]

comment:17

I do think however that we want to be able to pick up new packages without having to leave sage, don't we?

[jdemeyer]

comment:18

Replying to @mantepse:

Compiling the list once does not take very long.

Compiling the list once takes an extremely long time (almost 2 seconds on my machine).

[jm58660]

comment:19

Replying to @mantepse:

I do think however that we want to be able to pick up new packages without having to leave sage, don't we?

I don't think that it is an important feature. Would be nice, yes, but a minor issue.

[fchapoton]

Branch: u/chapoton/21289

[fchapoton]

Commit: 4d8b477

[sagetrac-git]

Changed commit from 4d8b477 to ec04bff

[sagetrac-git]

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

ec04bff

forgot another bliss usage

[jdemeyer]

comment:33

Could you replace

raise ImportError("You must install the 'bliss' package to run this command.")

by

from sage.misc.package import PackageNotFoundError
raise PackageNotFoundError("bliss")

This will give a customized error message.

[jdemeyer]

Author: Frédéric Chapoton

[jdemeyer]

Changed reviewer from Martin Rubey to Jeroen Demeyer

[sagetrac-git]

Changed commit from ec04bff to 253517d

[sagetrac-git]

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

253517d

using PackageNotFoundError twice

[fchapoton]

comment:35

done

[fchapoton]

comment:36

ping ?

[jdemeyer]

comment:37

This looks good, but it just needs testing that it works both without and with bliss.

[jdemeyer]

comment:38

Let's wait for the patchbot...

[fchapoton]

comment:39

ok, bot is green. Jeroen ?

[jdemeyer]

comment:40

I'll test it now on my computer with bliss installed.

[kiwifb]

comment:41

Given that this code is very close to the stuff I do in sage-on-gentoo with that file - may be Jeroen has been inspired by something he saw a while back :P - I would put this positive. But since Jeroen wants to try it, I'll let him push the button.

[jdemeyer]

Changed reviewer from Jeroen Demeyer to François Bissey, Jeroen Demeyer

[vbraun]

Changed branch from u/chapoton/21289 to 253517d