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

Make lazy_import more friendly to pyflakes and other static checkers #30647

Open
mkoeppe opened this issue Sep 23, 2020 · 28 comments
Open

Make lazy_import more friendly to pyflakes and other static checkers #30647

mkoeppe opened this issue Sep 23, 2020 · 28 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 23, 2020

pyflakes does not know about lazy_import and may therefore emit distracting warnings - as noted in #30616 comment:8

We introduce a new method PythonModule.lazy_import, which enables the following new idiom for creating lazy imports that are visible to static checkers:

LibBraiding = PythonModule('sage.libs.braiding', spkg='libbraiding')
rightnormalform = LibBraiding.lazy_import(
  'rightnormalform', namespace=None)
centralizer,  supersummitset = LibBraiding.lazy_import(
  ('centralizer', 'supersummitset'), namespace=None)

See also:

Depends on #30616

CC: @dcoudert @tobiasdiez @fchapoton @jhpalmieri @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/make_lazy_import_more_friendly_to_pyflakes_and_other_static_checkers @ 9b19c7a

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

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:2

As a workaround, you could use lazy imports in conjunction with traditional imports, but place the traditional imports within a if TYPE_CHECKING: statement. That way, it won't execute at runtime, but the type checker will know about import for type checking purposes.

(from microsoft/pyright#954 (comment))

@fchapoton
Copy link
Contributor

comment:3

Note that the patchbot "pyflakes plugin" knows about lazy imports.

BUT only with a basic syntax, one-liners.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:5

Replying to @tobiasdiez:

As a workaround, you could use lazy imports in conjunction with traditional imports, but place the traditional imports within a if TYPE_CHECKING: statement. That way, it won't execute at runtime, but the type checker will know about import for type checking purposes.

(from microsoft/pyright#954 (comment))

Thanks for the pointer!

Given that lazy imports are so widely used in sage, we probably don't want to use this workaround unless really necessary. It would add a lot of clutter to lots of files

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:6

A simple solution would be to make lazy_import return the (tuple of) lazy import objects that it creates, instead of relying on the magical namespace injection as a side effect. Then we could write

rightnormalform, centralizer, supersummitset = lazy_import(
  'sage.libs.braiding',
  ['rightnormalform', 'centralizer', 'supersummitset'],
  feature=PythonModule('sage.libs.braiding', spkg='libbraiding'))

Of course, we could as well just use LazyImport directly:

LibBraiding = PythonModule('sage.libs.braiding', spkg='libbraiding')
rightnormalform = LazyImport('sage.libs.braiding', 'rightnormalform', feature=LibBraiding)
centralizer     = LazyImport('sage.libs.braiding', 'centralizer',     feature=LibBraiding)
supersummitset  = LazyImport('sage.libs.braiding', 'supersummitset',  feature=LibBraiding)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:7

Expanding on this, we could introduce a method PythonModule.lazy_import that would enable writing this:

LibBraiding = PythonModule('sage.libs.braiding', spkg='libbraiding')
rightnormalform = LibBraiding.lazy_import('rightnormalform')
centralizer,  supersummitset = LibBraiding.lazy_import(
  'centralizer', 'supersummitset')

@tobiasdiez
Copy link
Contributor

comment:8

I think this will not be sufficient for the static typing analysis. You would need to provide the information that lazy_import('class') returns an instance of class.

Maybe one could declare it as lazy_import(Type[C] cls) -> C and call it via lazy_import(sage.libs.braiding.rightnormalform) (note that the argument is not a string). Not sure if that works.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

81207aelazy_import: Use namespace=True as the default; if namespace=None, return the LazyImport objects

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Commit: 81207ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Changed commit from 81207ae to 8922351

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

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

4793555sage.features.PythonModule.lazy_import: New
8922351src/sage/graphs/graph.py: Use PythonModule.lazy_import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

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

9b19c7asrc/sage/groups/braid.py: Use PythonModule.lazy_import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Changed commit from 8922351 to 9b19c7a

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:13

Here's something that seems to work.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:14

Replying to @tobiasdiez:

I think this will not be sufficient for the static typing analysis. You would need to provide the information that lazy_import('class') returns an instance of class.

In the current version, I suppose one could just use a typed assignment

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 23, 2020

comment:18

I was hoping for a syntax such as from sage.__lazy__.libs.braiding import rightnormalform. I think this can be implemented with the python3 import machinery (https://docs.python.org/3/reference/import.html#finders-and-loaders) but haven't investigated.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2020

Dependencies: #30616

@jhpalmieri
Copy link
Member

comment:20

Could you please document the feature argument in lazy_import.pyx?

@tobiasdiez
Copy link
Contributor

comment:21

Replying to @mkoeppe:

Replying to @tobiasdiez:

I think this will not be sufficient for the static typing analysis. You would need to provide the information that lazy_import('class') returns an instance of class.

In the current version, I suppose one could just use a typed assignment

What do you mean with typed assignment?
In the previous version,

try:
    from sage.graphs.mcqd import mcqd
except ImportError:
    from sage.misc.package import PackageNotFoundError
    raise PackageNotFoundError("mcqd")
return mcqd(self)

static type checkers can check if mcqd is actually a function defined in sage.graphs.mcqd and if self is allowed as an argument of mcqd. I doubt that they are intelligent enough to give the same information for the new version:

from sage.features import PythonModule
Mcqd = PythonModule('sage.graphs.mcqd', spkg='mcqd')
mcqd = Mcqd.lazy_import('mcqd', namespace=None)
return mcqd(self)

@tobiasdiez
Copy link
Contributor

comment:23

Another option would be to introduce (global) booleans that indicate whether a given library is available. E.g.

// e.g. in sage.distribution
def is_mcqd():
   try:
      from sage.graphs.mcqd import mcqd
      return true 
   except ImportError:
      return false
   // or some other way to determine if mcqd is available

// usage in another module:

// at the beginning: global import
if sage.distribution.is_mcqd():
    from sage.graphs.mcqd import mcqd
...
// later in the code
if sage.distribution.is_mcqd():
    return mcqd(self)
else:
    return ...

Yet another solution would be the following pattern:

try:
    from sage.graphs.mcqd import mcqd as _mcqd 
except ImportError:
    mcqd = None
else:
    mcqd = _mcqd

// later
if mcqd:
    return mcqd(self)
else:
    return ...

suggested in python/mypy#1297 (comment).
This should work with mypy, not sure about pyright.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2020

comment:24

Replying to @jhpalmieri:

Could you please document the feature argument in lazy_import.pyx?

This has now happened in #30587.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2020

comment:25

Replying to @mkoeppe:

I was hoping for a syntax such as from sage.__lazy__.libs.braiding import rightnormalform. I think this can be implemented with the python3 import machinery (https://docs.python.org/3/reference/import.html#finders-and-loaders) but haven't investigated.

This was previously discussed in #22752

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2022

comment:26

Another solution is to provide type stub files (__init__.pyi), see section "type checkers" in https://scientific-python.org/specs/spec-0001/

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