Opened 8 months ago
Closed 5 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:  sage9.3 
Component:  geometry  Keywords:  sd111 
Cc:  ghkliem, 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 7 months ago by
 Cc ghtobiasdiez added
comment:2 Changed 5 months ago by
 Keywords sd111 added
comment:3 Changed 5 months ago by
comment:4 Changed 5 months ago by
 Cc ghtobiasdiez removed
comment:5 Changed 5 months ago by
 Branch set to public/30587
 Commit set to c8cb27a7b85d28b8f73560a5c257b2e2265629ac
 Status changed from new to needs_review
comment:6 Changed 5 months ago by
 Status changed from needs_review to needs_work
 Work issues set to use feature of lazy import
comment:7 Changed 5 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 5 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 5 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 5 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 5 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 5 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 nonexistingpackage
will only appear on sagethedistribution but not in downstream packaged sage, so it's best to use ...
here
comment:13 Changed 5 months ago by
Ok, I wasn't exactly sure, what part would appear and what part would not.
comment:14 Changed 5 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 5 months ago by
In fact, perhaps LazyImport.__repr__
should catch this error and print itself in a friendlier way?
comment:16 Changed 5 months ago by
Also typo An example of and import
: and>an
comment:17 Changed 5 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 5 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 5 months ago by
I think the repl 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 5 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 5 months ago by
Hm... perhaps only for failed imports?
comment:22 Changed 5 months ago by
Well the other thing is lying as well. But sure.
comment:23 Changed 5 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 5 months ago by
patchbot shows errors
comment:25 Changed 5 months ago by
 Status changed from needs_review to needs_work
comment:26 Changed 5 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 5 months ago by
 Status changed from needs_work to needs_review
Somehow forgot that.
comment:28 Changed 5 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 5 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 5 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 nonexistingpackage No equivalent system packages for pip are known to Sage. **********************************************************************
comment:31 Changed 5 months ago by
Looks like we never tested the runtime system package advice on macOS without homebrew. unknown
needs specialcasing
comment:32 Changed 5 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 5 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 5 months ago by
 Reviewers changed from Matthias Koeppe to Matthias Koeppe, ...
 Status changed from needs_work to needs_review
comment:35 Changed 5 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 5 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.)