Opened 6 years ago

Closed 6 years ago

#21289 closed defect (fixed)

Don't use is_package_installed('bliss')

Reported by: Martin Rubey Owned by:
Priority: major Milestone: sage-7.4
Component: graph theory Keywords:
Cc: Jori Mäntysalo, Frédéric Chapoton, Jeroen Demeyer, Matthias Köppe, Travis Scrimshaw Merged in:
Authors: Frédéric Chapoton Reviewers: François Bissey, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 253517d (Commits, GitHub, GitLab) Commit: 253517d06c87cebbf597598bd5cf0a3cf97d4fcc
Dependencies: Stopgaps:

Status badges

Description

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

Change History (43)

comment:1 Changed 6 years ago by Jori Mäntysalo

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?

comment:2 Changed 6 years ago by Martin Rubey

(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.

comment:3 in reply to:  2 Changed 6 years ago by Jori Mäntysalo

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.

comment:4 Changed 6 years ago by Jori Mäntysalo

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.

comment:5 Changed 6 years ago by Jori Mäntysalo

Cc: Jeroen Demeyer added

Jeroen is propably interested in this.

comment:6 Changed 6 years ago by Jori Mäntysalo

Milestone: sage-7.4sage-duplicate/invalid/wontfix
Status: newneeds_review

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

comment:7 Changed 6 years ago by Martin Rubey

Status: needs_reviewpositive_review

comment:8 Changed 6 years ago by Martin Rubey

Reviewers: Martin Rubey

comment:9 in reply to:  2 Changed 6 years ago by Jeroen Demeyer

Milestone: sage-duplicate/invalid/wontfixsage-7.4
Status: positive_reviewneeds_work
Summary: speed regression in Poset.canonical_labelDon't use is_package_installed('bliss')

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.

comment:10 Changed 6 years ago by Martin Rubey

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

comment:11 Changed 6 years ago by Jori Mäntysalo

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

comment:12 in reply to:  11 ; Changed 6 years ago by Jeroen Demeyer

Replying to jmantysalo:

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.

comment:13 in reply to:  10 Changed 6 years ago by Martin Rubey

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...

comment:14 in reply to:  10 Changed 6 years ago by Jeroen Demeyer

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.

comment:15 in reply to:  12 Changed 6 years ago by Jori Mäntysalo

Replying to jdemeyer:

Replying to jmantysalo:

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?

comment:16 in reply to:  12 ; Changed 6 years ago by Martin Rubey

Replying to jdemeyer:

Replying to jmantysalo:

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.

comment:17 Changed 6 years ago by Martin Rubey

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

comment:18 in reply to:  16 ; Changed 6 years ago by Jeroen Demeyer

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).

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:19 in reply to:  17 Changed 6 years ago by Jori Mäntysalo

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.

comment:20 in reply to:  18 Changed 6 years ago by Jori Mäntysalo

Replying to jdemeyer:

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

OK. And there is no way to hook it to package installation?

comment:21 Changed 6 years ago by Matthias Köppe

Cc: Matthias Köppe added

comment:22 Changed 6 years ago by Jori Mäntysalo

Cc: Travis Scrimshaw added

FYI Travis: doctesting _kl_poly(x, y) takes now about 5 seconds because of this.

comment:23 Changed 6 years ago by Frédéric Chapoton

This is maybe (probably?) the cause of several annoying timeout failures seen in the patchbots, in particular in tutte_polynomial.py

comment:24 Changed 6 years ago by Jori Mäntysalo

Could we cache result of is_package_installed()? Or maybe result of pip_installed_packages() or something like that?

comment:25 in reply to:  24 Changed 6 years ago by Jeroen Demeyer

Replying to jmantysalo:

Could we cache result of is_package_installed()? Or maybe result of pip_installed_packages() or something like that?

All that is irrelevant to this ticket, which is just specifically about bliss.

comment:26 Changed 6 years ago by Jeroen Demeyer

See #20382 and #21291 for more general discussions.

comment:27 in reply to:  24 ; Changed 6 years ago by Jeroen Demeyer

Replying to jmantysalo:

Could we cache result of is_package_installed()?

As I already said in 18, that would not help.

comment:28 in reply to:  27 Changed 6 years ago by Jori Mäntysalo

Replying to jdemeyer:

Replying to jmantysalo:

Could we cache result of is_package_installed()?

As I already said in 18, that would not help.

?? If the result is cached, we got only one delay of two seconds. Currently running something like

g_list = [g.canonical_label() for g in g_list]

is extremely slow. Or maybe I miss something obvious.

comment:29 Changed 6 years ago by Jeroen Demeyer

This ticket is super-easy to fix: just apply what I said in 9 instead of coming up with overly-complicated solutions.

comment:30 Changed 6 years ago by Jeroen Demeyer

Just do something like:

try:
    import sage.graphs.bliss
    have_bliss = True
except ImportError:
    have_bliss = False

and use this have_bliss variable where needed.

That's it!

comment:31 Changed 6 years ago by Frédéric Chapoton

Branch: u/chapoton/21289
Commit: 4d8b477e76e1116eb41415da613a6529252de4c3
Status: needs_workneeds_review

done, please review


New commits:

4d8b477just test import bliss to see if it is available

comment:32 Changed 6 years ago by git

Commit: 4d8b477e76e1116eb41415da613a6529252de4c3ec04bff3df48f6199b658d709872e88460a4e014

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

ec04bffforgot another bliss usage

comment:33 Changed 6 years ago by Jeroen Demeyer

Authors: Frédéric Chapoton
Reviewers: Martin RubeyJeroen Demeyer
Status: needs_reviewneeds_work

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.

comment:34 Changed 6 years ago by git

Commit: ec04bff3df48f6199b658d709872e88460a4e014253517d06c87cebbf597598bd5cf0a3cf97d4fcc

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

253517dusing PackageNotFoundError twice

comment:35 Changed 6 years ago by Frédéric Chapoton

Status: needs_workneeds_review

done

comment:36 Changed 6 years ago by Frédéric Chapoton

ping ?

comment:37 Changed 6 years ago by Jeroen Demeyer

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

comment:38 Changed 6 years ago by Jeroen Demeyer

Let's wait for the patchbot...

comment:39 Changed 6 years ago by Frédéric Chapoton

ok, bot is green. Jeroen ?

comment:40 Changed 6 years ago by Jeroen Demeyer

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

comment:41 Changed 6 years ago by François Bissey

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.

comment:42 Changed 6 years ago by Jeroen Demeyer

Reviewers: Jeroen DemeyerFrançois Bissey, Jeroen Demeyer
Status: needs_reviewpositive_review

comment:43 Changed 6 years ago by Volker Braun

Branch: u/chapoton/21289253517d06c87cebbf597598bd5cf0a3cf97d4fcc
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.