Opened 6 years ago

Closed 6 years ago

#21291 closed defect (fixed)

speed regression in is_package_installed

Reported by: mantepse Owned by:
Priority: blocker Milestone: sage-7.4
Component: misc Keywords: slowness, doctesting timeout, timed out
Cc: jdemeyer, vdelecroix, saraedum, mkoeppe Merged in:
Authors: Vincent Delecroix Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: bfeead0 (Commits, GitHub, GitLab) Commit: bfeead0e6f34a51fd7c99d7409e18253effd0cef
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

In 6.9.beta5 I have:

sage: sage: timeit("is_package_installed('bliss')")
625 loops, best of 3: 163 µs per loop

whereas in 7.4.beta1

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

possibly related: #20382


From sage-release:

> Other than the Tutte polynomial one, I get two other slow doctests that
> are not marked with #long time. I can not confirm whether it is a
> regression as I don't keep my old version of sage.
>
>
> sage -t --long --warn-long 31.5 src/sage/graphs/generators/families.py
> **********************************************************************
> File "src/sage/graphs/generators/families.py", line 2053, in
> sage.graphs.generators.families.petersen_family
> Warning, slow doctest:
>     F2 = graphs.petersen_family(generate=True)
> Test ran for 99.31 s
>     [280 tests, 128.07 s]
>
>
> sage -t --long --warn-long 31.5 src/sage/combinat/posets/posets.py
> **********************************************************************
> File "src/sage/combinat/posets/posets.py", line 6430, in
> sage.combinat.posets.posets.FinitePoset._kl_poly
> Warning, slow doctest:
>     L._kl_poly()
> Test ran for 127.72 s
>     [1122 tests, 146.65 s]

Both keep calling 'pip list' as well (same issue as with Tutte polynomial).

Change History (40)

comment:1 Changed 6 years ago by jdemeyer

I guess this is because of the PIP stuff which got added in #19213. I see no easy fix... the only possibility is removing the use of PIP in is_package_installed.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:2 follow-up: Changed 6 years ago by mantepse

I wonder why no speed regression was detected in #19213.

comment:3 in reply to: ↑ 2 Changed 6 years ago by jdemeyer

Replying to mantepse:

I wonder why no speed regression was detected in #19213.

Because nobody checked?

comment:4 follow-up: Changed 6 years ago by mantepse

Would it perhaps be possible to recompute the list of installed packages only when a new package is installed? Or, if that's not possible, just provide a user level function that recomputes the list of installed packages?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by jdemeyer

Replying to mantepse:

only when a new package is installed?

How are you going to detect whether a new package is installed without listing the installed packages?

comment:6 in reply to: ↑ 5 Changed 6 years ago by mantepse

Replying to jdemeyer:

Replying to mantepse:

only when a new package is installed?

How are you going to detect whether a new package is installed without listing the installed packages?

I don't know, that's why I asked.

comment:7 Changed 6 years ago by vdelecroix

See also trouble from compiling a fresh clone at https://groups.google.com/forum/#!topic/sage-release/MhAwPVWfK28

comment:8 Changed 6 years ago by jdemeyer

  • Priority changed from major to blocker

comment:9 Changed 6 years ago by mkoeppe

  • Cc mkoeppe added

comment:10 Changed 6 years ago by leif

See also #21355 which presumably is caused by the same.

How could such crap get merged?

comment:11 Changed 6 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/21291
  • Commit set to 1da650dbe6c32bd52876110074c1ee2aa6a49237
  • Status changed from new to needs_review

New commits:

1da650d21291: remove pip type from is_package_installed

comment:12 Changed 6 years ago by leif

I thought of (temporarily anyway, because of #20382) adding some keyword (default: False) like update or include_pip_packages, passing that in addition where/when appropriate.

That way we wouldn't completely exclude "pip" packages elsewhere.

comment:13 Changed 6 years ago by leif

IMHO installed (or some related variable) should be a dictionary mapping just "base" package names to {True, False}, i.e., a set().

Then is_package_installed() could just return package in WHATEVER (unless also update=True was passed, or the variable is yet None).

The package versions could be recorded in separate dictionaries.

comment:14 Changed 6 years ago by git

  • Commit changed from 1da650dbe6c32bd52876110074c1ee2aa6a49237 to 9dd83b8d8964971fc628a7b7f6e487df1963341b

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

9dd83b821291: pip_exclude option

comment:15 Changed 6 years ago by vdelecroix

Cleaner. But not one-line patch...

comment:16 follow-up: Changed 6 years ago by jdemeyer

Typo: Trure -> True

comment:17 in reply to: ↑ 16 Changed 6 years ago by leif

Replying to jdemeyer:

Typo: Trure -> True

There's also a colon missing after default.


Isn't the description of exclude_pip in list_packages() as wrong as that of local?

If the former is True (not False), PIP packages are excluded (not considered), or keep the False, and remove the "not" in "then [also] pip packages are not considered.".

Similar for the latter (local): Either change False to True, or remove the "not".

comment:18 Changed 6 years ago by git

  • Commit changed from 9dd83b8d8964971fc628a7b7f6e487df1963341b to ff7253eecdc91210e19135ad92627bdc89a3e5f5

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

ff7253e21291: fix documentation

comment:19 Changed 6 years ago by leif

Next typo (in before):

s/if set to True than connection error/if set to True then connection error/


According to the current Sage Developer's Guide, the syntax for arguments is

INPUT:

- ``foo`` (default: ``'bar'``) -- I won't describe its meaning here

that is, with a colon after default, and a double-dash before the description.

comment:20 Changed 6 years ago by leif

  • Description modified (diff)
  • Keywords slowness doctesting timeout timed out added

comment:21 follow-up: Changed 6 years ago by tscrim

*cough cough*

INPUT:

- ``foo`` -- (default: ``'bar'``) I won't describe its meaning here

comment:22 in reply to: ↑ 21 Changed 6 years ago by leif

Replying to tscrim:

*cough cough*

INPUT:

- ``foo`` -- (default: ``'bar'``) I won't describe its meaning here

Ouch. m)

comment:23 Changed 6 years ago by git

  • Commit changed from ff7253eecdc91210e19135ad92627bdc89a3e5f5 to 19284b3e3da5df6fe38601677b7fc379d9eff4d2

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

19284b321291: more fix to doc

comment:24 Changed 6 years ago by git

  • Commit changed from 19284b3e3da5df6fe38601677b7fc379d9eff4d2 to f766cb832b80820a1d81c03245a881f8ae3a758c

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

f766cb821291: more fix to doc

comment:25 follow-up: Changed 6 years ago by vdelecroix

the doc is changed (but I did not receive mail for that)

comment:26 in reply to: ↑ 25 Changed 6 years ago by leif

Replying to vdelecroix:

the doc is changed (but I did not receive mail for that)

Me neither, but I almost never get notifications for "git comments" (i.e., when someone just pushes).

comment:27 follow-up: Changed 6 years ago by leif

The "than no error is raised" is still in.

Also, in "a sublist of 'standard', 'optional', ..." the package type strings should have code markup (``'standard'`` etc.).

Similar further down in package_versions(); there, math markup (only single backticks) is used instead of double backticks.


(Second attempt to send this after an internal server error.)

comment:28 follow-up: Changed 6 years ago by leif

src/sage_setup/optional_extension.py uses list_packages() (which currently has exclude_pip=False as default); should we "fix" the former, too, and if so, here?

(Changing the default in list_packages() is perhaps too backward-incompatible, or we'd at least have to change a couple of calls elsewhere I guess.)


In principle, any pip-related function here should first check whether is_package_installed("pip")... ;-)

(There currently seems to be a dependency missing at least, since pip meanwhile is a standard package. Not sure whether we'll get cyclic dependencies then, though...)

comment:29 in reply to: ↑ 28 Changed 6 years ago by leif

Replying to leif:

src/sage_setup/optional_extension.py uses list_packages() (which currently has exclude_pip=False as default); should we "fix" the former, too, and if so, here?

(Changing the default in list_packages() is perhaps too backward-incompatible, or we'd at least have to change a couple of calls elsewhere I guess.)


In principle, any pip-related function here should first check whether is_package_installed("pip")... ;-)

(There currently seems to be a dependency missing at least, since pip meanwhile is a standard package. Not sure whether we'll get cyclic dependencies then, though...)

Fixed by #21403 (adding the missing dependency on pip); will appear in 7.4.beta4 I guess.

comment:30 Changed 6 years ago by git

  • Commit changed from f766cb832b80820a1d81c03245a881f8ae3a758c to c7f2ff52a2f11d9da18a22cba98b889eca5aec37

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

c7f2ff521291: fix doc

comment:31 follow-up: Changed 6 years ago by vdelecroix

doc is fixed

comment:32 in reply to: ↑ 31 Changed 6 years ago by leif

Replying to vdelecroix:

doc is fixed

No, you just made it worse... ;-)

I only meant replace the misspelled "than" by "then".

Now the "than" is still in, and the description is wrong, as ignore_URLError=True of course means don't raise an exception but simply return None on errors, but you turned the "no error" into "an error" and added another logically wrong sentence.

comment:33 in reply to: ↑ 27 Changed 6 years ago by leif

Replying to leif:

Also, in "a sublist of 'standard', 'optional', ..." the package type strings should have code markup (``'standard'`` etc.).

Similar further down in package_versions(); there, math markup (only single backticks) is used instead of double backticks.

This is also still true.

comment:34 Changed 6 years ago by git

  • Commit changed from c7f2ff52a2f11d9da18a22cba98b889eca5aec37 to 39860d24b0be882939223cdac115fd9fc54c8c01

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

39860d221291: fix doc

comment:35 Changed 6 years ago by vdelecroix

Indeed! (fixed, I hope)

comment:36 Changed 6 years ago by leif

I'll push a branch with the fixes later if you don't have the time.

It's still a blocker ticket...

Oh, wait, just seeing you've pushed a new one...

comment:37 Changed 6 years ago by leif

  • Reviewers set to Leif Leonhardy

So I've fixed more docstrings on top of your changes.

Either check out my branch u/leif/21291, or simply set the ticket to positive review.

For beta4, it's unfortunately meanwhile too late.

comment:38 Changed 6 years ago by vdelecroix

  • Branch changed from u/vdelecroix/21291 to u/leif/21291
  • Commit changed from 39860d24b0be882939223cdac115fd9fc54c8c01 to bfeead0e6f34a51fd7c99d7409e18253effd0cef
  • Status changed from needs_review to positive_review

New commits:

bfeead0Trac #21291: Fix more docstrings in misc/package.py

comment:39 Changed 6 years ago by vdelecroix

Thanks!

comment:40 Changed 6 years ago by vbraun

  • Branch changed from u/leif/21291 to bfeead0e6f34a51fd7c99d7409e18253effd0cef
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.