Opened 7 months ago

Closed 4 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:

Status badges

Description (last modified by gh-kliem)

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 mkoeppe

  • Cc gh-tobiasdiez added

comment:2 Changed 4 months ago by mkoeppe

  • Keywords sd111 added

comment:3 Changed 4 months ago by gh-kliem

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 from ppl.

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 into backend_ppl.py. Sage is starting all right, but polyhedra are unusable. However, only backend ppl should fail.)

comment:4 Changed 4 months ago by gh-tobiasdiez

  • Cc gh-tobiasdiez removed

comment:5 Changed 4 months ago by gh-kliem

  • Authors set to Jonathan Kliem
  • Branch set to public/30587
  • Commit set to c8cb27a7b85d28b8f73560a5c257b2e2265629ac
  • Status changed from new to needs_review

New commits:

7dae2c0lazy import polyhedra instances
736f7aelazy import ppl for cone
c8cb27aremove import of ppl through lattice polytope

comment:6 Changed 4 months ago by gh-kliem

  • Status changed from needs_review to needs_work
  • Work issues set to use feature of lazy import

comment:7 Changed 4 months ago by mjo

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 4 months ago by git

  • Commit changed from c8cb27a7b85d28b8f73560a5c257b2e2265629ac to d8934c06ca64f8402153c7e038f987883455db4e

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

d8934c0document feature of lazy import

comment:9 Changed 4 months ago by gh-kliem

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:

d8934c0document feature of lazy import

comment:10 Changed 4 months ago by git

  • Commit changed from d8934c06ca64f8402153c7e038f987883455db4e to 5feb98c3ebf754f6db2e26e1eced1621d8a6a163

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

5e53333Revert "lazy import polyhedra instances"
aca03b0using feature of lazy import for backend normaliz
8bd41ealazy import ppl in backend_ppl
a456030add feature to cone
19bc7e4add feature option to lattice polytope
5feb98cmissing import

comment:11 Changed 4 months ago by gh-kliem

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues use feature of lazy import deleted

comment:12 Changed 4 months ago by mkoeppe

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 4 months ago by gh-kliem

Ok, I wasn't exactly sure, what part would appear and what part would not.

comment:14 Changed 4 months ago by git

  • Commit changed from 5feb98c3ebf754f6db2e26e1eced1621d8a6a163 to d7a2de9bcb8d20df32fd0630e3ee3a9d5a049f74

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

d7a2de9make the doctest work outside of the distribution

comment:15 Changed 4 months ago by mkoeppe

In fact, perhaps LazyImport.__repr__ should catch this error and print itself in a friendlier way?

comment:16 Changed 4 months ago by mkoeppe

Also typo An example of and import : and->an

comment:17 Changed 4 months ago by mkoeppe

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 4 months ago by git

  • Commit changed from d7a2de9bcb8d20df32fd0630e3ee3a9d5a049f74 to 96f151a9ea4f4fd10607153bee0fb8bf609ce7dd

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

96f151anicer representation of not available module

comment:19 Changed 4 months ago by mkoeppe

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.

Last edited 4 months ago by mkoeppe (previous) (diff)

comment:20 Changed 4 months ago by git

  • Commit changed from 96f151a9ea4f4fd10607153bee0fb8bf609ce7dd to 044ae4389bec751bbfaf45a20284de6c02c6a284

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

044ae43lazy import representation acknowledges its type

comment:21 Changed 4 months ago by mkoeppe

Hm... perhaps only for failed imports?

comment:22 Changed 4 months ago by gh-kliem

Well the other thing is lying as well. But sure.

comment:23 Changed 4 months ago by git

  • Commit changed from 044ae4389bec751bbfaf45a20284de6c02c6a284 to b3ec686f4e557bf7e2901ef489300d330b0436ec

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

b3ec686only be verbose on failure

comment:24 Changed 4 months ago by mkoeppe

patchbot shows errors

comment:25 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:26 Changed 4 months ago by git

  • Commit changed from b3ec686f4e557bf7e2901ef489300d330b0436ec to 3c7b704e2bbb421a074dfd6c3e977be25ba24d87

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

3c7b704lazily import PyNormaliz

comment:27 Changed 4 months ago by gh-kliem

  • Status changed from needs_work to needs_review

Somehow forgot that.

comment:28 Changed 4 months ago by mkoeppe

  • 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 4 months ago by gh-kliem

Thank you. So I guess I know how that mechanism works, you can ping me, to do it with other packages.

comment:30 Changed 4 months ago by vbraun

  • 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 4 months ago by mkoeppe

Looks like we never tested the runtime system package advice on macOS without homebrew. unknown needs special-casing

comment:32 Changed 4 months ago by git

  • Commit changed from 3c7b704e2bbb421a074dfd6c3e977be25ba24d87 to 81a655efd3e477df5dddad31fc26c5e6ac23439c

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

81a655esage.features.package_systems: Ignore 'unknown'

comment:33 Changed 4 months ago by git

  • Commit changed from 81a655efd3e477df5dddad31fc26c5e6ac23439c to 17c4cdb90244a584e7ee5d98796877d06bfae009

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

17c4cdbsage.misc.lazy_import: Add some more '...' to doctest

comment:34 Changed 4 months ago by mkoeppe

  • Authors changed from Jonathan Kliem to Jonathan Kliem, Matthias Koeppe
  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, ...
  • Status changed from needs_work to needs_review

comment:35 Changed 4 months ago by gh-kliem

  • 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 4 months ago by vbraun

  • Branch changed from public/30587 to 17c4cdb90244a584e7ee5d98796877d06bfae009
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.