Opened 23 months ago
Closed 20 months ago
#30587 closed enhancement (fixed)
Remove import of 'ppl' at startup using lazy_import with feature
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | geometry | Keywords: | sd111 |
Cc: | gh-kliem, novoselt, vbraun, Winfried, jipilab, mjo, vdelecroix | Merged in: | |
Authors: | Jonathan Kliem, Matthias Koeppe | Reviewers: | Matthias Koeppe, Jonathan Kliem |
Report Upstream: | N/A | Work issues: | |
Branch: | 17c4cdb (Commits, GitHub, GitLab) | Commit: | 17c4cdb90244a584e7ee5d98796877d06bfae009 |
Dependencies: | Stopgaps: |
Description (last modified by )
Currently, sage imports ppl
(from pplpy
) at startup, via sage.geometry.cone
, sage.geometry.integral_points
, and sage.geometry.lattice_polytopes
.
This should be changed, both
- as preparation for other backends for polyhedral cones (in particular
PyNormaliz
) - for the modularization of sagelib (#29705).
In addition we add documentation to the option feature
of lazy import and while we are at it, use this option for backend normaliz as well.
Change History (36)
comment:1 Changed 23 months ago by
- Cc gh-tobiasdiez added
comment:2 Changed 20 months ago by
- Keywords sd111 added
comment:3 Changed 20 months ago by
comment:4 Changed 20 months ago by
- Cc gh-tobiasdiez removed
comment:5 Changed 20 months ago by
- Branch set to public/30587
- Commit set to c8cb27a7b85d28b8f73560a5c257b2e2265629ac
- Status changed from new to needs_review
comment:6 Changed 20 months ago by
- Status changed from needs_review to needs_work
- Work issues set to use feature of lazy import
comment:7 Changed 20 months ago by
If the imports would not wind up in an extremely hot path, you might also work around the circular imports with local ones (from foo import whatever
within the function that needs whatever
). Those don't get run until the function is called, but it does introduce a tiny redundant dictionary lookup the second, third, etc times the function is called.
comment:8 Changed 20 months ago by
- Commit changed from c8cb27a7b85d28b8f73560a5c257b2e2265629ac to d8934c06ca64f8402153c7e038f987883455db4e
Branch pushed to git repo; I updated commit sha1. New commits:
d8934c0 | document feature of lazy import
|
comment:9 Changed 20 months ago by
So lazy import appears to be a bit slower.
For %timeit polytopes.cube()
I went from 411 µs to 417. I don't know if this is acceptable.
So there appears to be a overhead of about 6 µs per call.
New commits:
d8934c0 | document feature of lazy import
|
comment:10 Changed 20 months ago by
- Commit changed from d8934c06ca64f8402153c7e038f987883455db4e to 5feb98c3ebf754f6db2e26e1eced1621d8a6a163
Branch pushed to git repo; I updated commit sha1. New commits:
5e53333 | Revert "lazy import polyhedra instances"
|
aca03b0 | using feature of lazy import for backend normaliz
|
8bd41ea | lazy import ppl in backend_ppl
|
a456030 | add feature to cone
|
19bc7e4 | add feature option to lattice polytope
|
5feb98c | missing import
|
comment:11 Changed 20 months ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues use feature of lazy import deleted
comment:12 Changed 20 months ago by
In the doctest for lazy_import
, this text:
+ To install foo using the Sage package manager, you can try to run: + !sage -i non-existing-package
will only appear on sage-the-distribution but not in downstream packaged sage, so it's best to use ...
here
comment:13 Changed 20 months ago by
Ok, I wasn't exactly sure, what part would appear and what part would not.
comment:14 Changed 20 months ago by
- Commit changed from 5feb98c3ebf754f6db2e26e1eced1621d8a6a163 to d7a2de9bcb8d20df32fd0630e3ee3a9d5a049f74
Branch pushed to git repo; I updated commit sha1. New commits:
d7a2de9 | make the doctest work outside of the distribution
|
comment:15 Changed 20 months ago by
In fact, perhaps LazyImport.__repr__
should catch this error and print itself in a friendlier way?
comment:16 Changed 20 months ago by
Also typo An example of and import
: and->an
comment:17 Changed 20 months ago by
Another data point regarding performance: Without this ticket, I am getting
sage: %timeit Polyhedron(vertices=[[]]) 51.7 µs ± 866 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each) sage: %timeit Polyhedron(vertices=[[]]) 53.9 µs ± 774 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each) sage: %timeit Polyhedron(vertices=[[]]) 52.8 µs ± 738 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
Right after building this branch, I am getting:
sage: %timeit Polyhedron(vertices=[[]]) 88.2 µs ± 22.3 µs per loop (mean ± std. dev. of 7 runs, 1 loop each) sage: %timeit Polyhedron(vertices=[[]]) 57.2 µs ± 1.22 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each) sage: %timeit Polyhedron(vertices=[[]]) 53.5 µs ± 307 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each) sage: %timeit Polyhedron(vertices=[[]]) 53.2 µs ± 508 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
So I don't think that we should be concerned about the performance impact here.
comment:18 Changed 20 months ago by
- Commit changed from d7a2de9bcb8d20df32fd0630e3ee3a9d5a049f74 to 96f151a9ea4f4fd10607153bee0fb8bf609ce7dd
Branch pushed to git repo; I updated commit sha1. New commits:
96f151a | nicer representation of not available module
|
comment:19 Changed 20 months ago by
I think the repr should express somehow that it is a LazyImport
object so that users who see this output can know where this is coming from.
comment:20 Changed 20 months ago by
- Commit changed from 96f151a9ea4f4fd10607153bee0fb8bf609ce7dd to 044ae4389bec751bbfaf45a20284de6c02c6a284
Branch pushed to git repo; I updated commit sha1. New commits:
044ae43 | lazy import representation acknowledges its type
|
comment:21 Changed 20 months ago by
Hm... perhaps only for failed imports?
comment:22 Changed 20 months ago by
Well the other thing is lying as well. But sure.
comment:23 Changed 20 months ago by
- Commit changed from 044ae4389bec751bbfaf45a20284de6c02c6a284 to b3ec686f4e557bf7e2901ef489300d330b0436ec
Branch pushed to git repo; I updated commit sha1. New commits:
b3ec686 | only be verbose on failure
|
comment:24 Changed 20 months ago by
patchbot shows errors
comment:25 Changed 20 months ago by
- Status changed from needs_review to needs_work
comment:26 Changed 20 months ago by
- Commit changed from b3ec686f4e557bf7e2901ef489300d330b0436ec to 3c7b704e2bbb421a074dfd6c3e977be25ba24d87
Branch pushed to git repo; I updated commit sha1. New commits:
3c7b704 | lazily import PyNormaliz
|
comment:27 Changed 20 months ago by
- Status changed from needs_work to needs_review
Somehow forgot that.
comment:28 Changed 20 months ago by
- Reviewers set to Matthias Koeppe
- Status changed from needs_review to positive_review
- Summary changed from Remove import of 'ppl' at startup to Remove import of 'ppl' at startup using lazy_import with feature
comment:29 Changed 20 months ago by
Thank you. So I guess I know how that mechanism works, you can ping me, to do it with other packages.
comment:30 Changed 20 months ago by
- Status changed from positive_review to needs_work
On OSX:
********************************************************************** File "src/sage/misc/lazy_import.pyx", line 1073, in sage.misc.lazy_import.? Failed example: not_there Expected: Failed lazy import: foo is not available. Importing not_there failed: No module named 'foo' No equivalent system packages for ... are known to Sage. ... Got: Failed lazy import: foo is not available. Importing not_there failed: No module named 'foo' To install foo using the unknown package manager, you can try to run: # install the following packages: To install foo using the Sage package manager, you can try to run: !sage -i non-existing-package No equivalent system packages for pip are known to Sage. **********************************************************************
comment:31 Changed 20 months ago by
Looks like we never tested the runtime system package advice on macOS without homebrew. unknown
needs special-casing
comment:32 Changed 20 months ago by
- Commit changed from 3c7b704e2bbb421a074dfd6c3e977be25ba24d87 to 81a655efd3e477df5dddad31fc26c5e6ac23439c
Branch pushed to git repo; I updated commit sha1. New commits:
81a655e | sage.features.package_systems: Ignore 'unknown'
|
comment:33 Changed 20 months ago by
- Commit changed from 81a655efd3e477df5dddad31fc26c5e6ac23439c to 17c4cdb90244a584e7ee5d98796877d06bfae009
Branch pushed to git repo; I updated commit sha1. New commits:
17c4cdb | sage.misc.lazy_import: Add some more '...' to doctest
|
comment:34 Changed 20 months ago by
- Reviewers changed from Matthias Koeppe to Matthias Koeppe, ...
- Status changed from needs_work to needs_review
comment:35 Changed 20 months ago by
- Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem
- Status changed from needs_review to positive_review
Thanks for the quick fix.
comment:36 Changed 20 months ago by
- Branch changed from public/30587 to 17c4cdb90244a584e7ee5d98796877d06bfae009
- Resolution set to fixed
- Status changed from positive_review to closed
I'm looking at this at the moment.
I think one needs to also take case of
Polyhedron_base
, because once it is lazily imported, then it immediately imports all depencies, in particular the stuff fromppl
.So if the goal is to have a working sage as good as possible without
ppl
, then this should be touched as well.(I tested adding a line
from foo import nonsense
intobackend_ppl.py
. Sage is starting all right, but polyhedra are unusable. However, only backend ppl should fail.)