Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove import of 'ppl' at startup using lazy_import with feature #30587

Closed
mkoeppe opened this issue Sep 16, 2020 · 48 comments
Closed

Remove import of 'ppl' at startup using lazy_import with feature #30587

mkoeppe opened this issue Sep 16, 2020 · 48 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Sep 16, 2020

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

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.

CC: @kliem @novoselt @vbraun @w-bruns @jplab @orlitzky @videlec

Component: geometry

Keywords: sd111

Author: Jonathan Kliem, Matthias Koeppe

Branch/Commit: 17c4cdb

Reviewer: Matthias Koeppe, Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/30587

@mkoeppe mkoeppe added this to the sage-9.3 milestone Sep 16, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@kliem
Copy link
Contributor

kliem commented Dec 7, 2020

comment:3

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

@kliem
Copy link
Contributor

kliem commented Dec 7, 2020

New commits:

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

@kliem
Copy link
Contributor

kliem commented Dec 7, 2020

Commit: c8cb27a

@kliem
Copy link
Contributor

kliem commented Dec 7, 2020

Author: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Dec 7, 2020

Branch: public/30587

@kliem
Copy link
Contributor

kliem commented Dec 9, 2020

Work Issues: use feature of lazy import

@orlitzky
Copy link
Contributor

orlitzky commented Dec 9, 2020

comment:7

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

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

d8934c0document feature of lazy import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

Changed commit from c8cb27a to d8934c0

@kliem
Copy link
Contributor

kliem commented Dec 10, 2020

comment:9

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

Changed commit from d8934c0 to 5feb98c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

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

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor

kliem commented Dec 10, 2020

Changed work issues from use feature of lazy import to none

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 10, 2020

comment:12

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

@kliem
Copy link
Contributor

kliem commented Dec 10, 2020

comment:13

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

Changed commit from 5feb98c to d7a2de9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

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

d7a2de9make the doctest work outside of the distribution

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 10, 2020

comment:15

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 10, 2020

comment:16

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 11, 2020

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

3c7b704lazily import PyNormaliz

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 11, 2020

Changed commit from b3ec686 to 3c7b704

@kliem
Copy link
Contributor

kliem commented Dec 11, 2020

comment:27

Somehow forgot that.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 11, 2020

Reviewer: Matthias Koeppe

@mkoeppe mkoeppe changed the title Remove import of 'ppl' at startup Remove import of 'ppl' at startup using lazy_import with feature Dec 11, 2020
@kliem
Copy link
Contributor

kliem commented Dec 11, 2020

comment:29

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

@vbraun
Copy link
Member

vbraun commented Dec 13, 2020

comment:30

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2020

comment:31

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2020

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

81a655esage.features.package_systems: Ignore 'unknown'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2020

Changed commit from 3c7b704 to 81a655e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 13, 2020

Changed commit from 81a655e to 17c4cdb

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2020

Changed author from Jonathan Kliem to Jonathan Kliem, Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2020

Changed reviewer from Matthias Koeppe to Matthias Koeppe, ...

@kliem
Copy link
Contributor

kliem commented Dec 13, 2020

comment:35

Thanks for the quick fix.

@kliem
Copy link
Contributor

kliem commented Dec 13, 2020

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem

@vbraun
Copy link
Member

vbraun commented Dec 14, 2020

Changed branch from public/30587 to 17c4cdb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants