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: |
Description (last modified by )
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
comment:2 follow-up: ↓ 3 Changed 6 years ago by
I wonder why no speed regression was detected in #19213.
comment:3 in reply to: ↑ 2 Changed 6 years ago by
comment:4 follow-up: ↓ 5 Changed 6 years ago by
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: ↓ 6 Changed 6 years ago by
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
comment:7 Changed 6 years ago by
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
- Priority changed from major to blocker
comment:9 Changed 6 years ago by
- Cc mkoeppe added
comment:10 Changed 6 years ago by
See also #21355 which presumably is caused by the same.
How could such crap get merged?
comment:11 Changed 6 years ago by
- Branch set to u/vdelecroix/21291
- Commit set to 1da650dbe6c32bd52876110074c1ee2aa6a49237
- Status changed from new to needs_review
New commits:
1da650d | 21291: remove pip type from is_package_installed
|
comment:12 Changed 6 years ago by
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
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
- Commit changed from 1da650dbe6c32bd52876110074c1ee2aa6a49237 to 9dd83b8d8964971fc628a7b7f6e487df1963341b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9dd83b8 | 21291: pip_exclude option
|
comment:15 Changed 6 years ago by
Cleaner. But not one-line patch...
comment:16 follow-up: ↓ 17 Changed 6 years ago by
Typo: Trure
-> True
comment:17 in reply to: ↑ 16 Changed 6 years ago by
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
- Commit changed from 9dd83b8d8964971fc628a7b7f6e487df1963341b to ff7253eecdc91210e19135ad92627bdc89a3e5f5
Branch pushed to git repo; I updated commit sha1. New commits:
ff7253e | 21291: fix documentation
|
comment:19 Changed 6 years ago by
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
- Description modified (diff)
- Keywords slowness doctesting timeout timed out added
comment:21 follow-up: ↓ 22 Changed 6 years ago by
*cough cough*
INPUT: - ``foo`` -- (default: ``'bar'``) I won't describe its meaning here
comment:22 in reply to: ↑ 21 Changed 6 years ago by
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
- Commit changed from ff7253eecdc91210e19135ad92627bdc89a3e5f5 to 19284b3e3da5df6fe38601677b7fc379d9eff4d2
Branch pushed to git repo; I updated commit sha1. New commits:
19284b3 | 21291: more fix to doc
|
comment:24 Changed 6 years ago by
- Commit changed from 19284b3e3da5df6fe38601677b7fc379d9eff4d2 to f766cb832b80820a1d81c03245a881f8ae3a758c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f766cb8 | 21291: more fix to doc
|
comment:25 follow-up: ↓ 26 Changed 6 years ago by
the doc is changed (but I did not receive mail for that)
comment:26 in reply to: ↑ 25 Changed 6 years ago by
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: ↓ 33 Changed 6 years ago by
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: ↓ 29 Changed 6 years ago by
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
Replying to leif:
src/sage_setup/optional_extension.py
useslist_packages()
(which currently hasexclude_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
- Commit changed from f766cb832b80820a1d81c03245a881f8ae3a758c to c7f2ff52a2f11d9da18a22cba98b889eca5aec37
Branch pushed to git repo; I updated commit sha1. New commits:
c7f2ff5 | 21291: fix doc
|
comment:31 follow-up: ↓ 32 Changed 6 years ago by
doc is fixed
comment:32 in reply to: ↑ 31 Changed 6 years ago by
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
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
- Commit changed from c7f2ff52a2f11d9da18a22cba98b889eca5aec37 to 39860d24b0be882939223cdac115fd9fc54c8c01
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
39860d2 | 21291: fix doc
|
comment:35 Changed 6 years ago by
Indeed! (fixed, I hope)
comment:36 Changed 6 years ago by
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
- 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
- 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:
bfeead0 | Trac #21291: Fix more docstrings in misc/package.py
|
comment:39 Changed 6 years ago by
Thanks!
comment:40 Changed 6 years ago by
- Branch changed from u/leif/21291 to bfeead0e6f34a51fd7c99d7409e18253effd0cef
- Resolution set to fixed
- Status changed from positive_review to closed
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
.