speed regression in is_package_installed

[mantepse]

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

[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 is is_package_installed.

[mantepse]

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

[jdemeyer]

Replying to mantepse:

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

Because nobody checked?

[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?

[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?

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

[vdelecroix]

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

[leif]

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

How could such crap get merged?

[vdelecroix]

New commits:

​1da650d

21291: remove pip type from is_package_installed

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

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

[git]

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

​9dd83b8

21291: pip_exclude option

[vdelecroix]

Cleaner. But not one-line patch...

[jdemeyer]

Typo: Trure -> True

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

[git]

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

​ff7253e

21291: fix documentation

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

[tscrim]

*cough cough*

INPUT:

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

[leif]

Replying to tscrim:

*cough cough*

INPUT:

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

Ouch. m)

[git]

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

​19284b3

21291: more fix to doc

[git]

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

​f766cb8

21291: more fix to doc

[vdelecroix]

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

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

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

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

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

[git]

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

​c7f2ff5

21291: fix doc

[vdelecroix]

doc is fixed

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

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

[git]

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

​39860d2

21291: fix doc

[vdelecroix]

Indeed! (fixed, I hope)

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

[leif]

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.

[vdelecroix]

New commits:

​bfeead0

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

[vdelecroix]

Thanks!