Opened 9 years ago

Closed 9 years ago

#14767 closed defect (fixed)

Fix import_statements for lazy imported objects

Reported by: Thierry Monteil Owned by: Jason Grout
Priority: major Milestone: sage-5.11
Component: misc Keywords: days49
Cc: Vincent Delecroix Merged in: sage-5.11.rc0
Authors: Vincent Delecroix Reviewers: Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Nicolas M. Thiéry)

Here is an example:

sage: import_statements('NN')
from sage.misc.lazy_import import NN
sage: from sage.misc.lazy_import import NN
...
ImportError: cannot import name NN

That's because NN is a not-yet-resolved lazy imported object.

Solution: if the object is a LazyImport then get the corresponding object with ._get_object. With the patch applied:

sage: import_statements(NN)
from sage.rings.semirings.non_negative_integer_semiring import NN
sage: import_statements('NN')
from sage.rings.semirings.non_negative_integer_semiring import NN

A potential bonus feature would have been to set lazy=True by default for lazy imported object:

sage: import_statements(NN)
lazy_import('from sage.rings.semirings.non_negative_integer_semiring', 'NN')

but the idea was dropped.

Attachments (1)

trac_14767.patch (1.3 KB) - added by Vincent Delecroix 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by Nicolas M. Thiéry

Cc: Vincent Delecroix added
Description: modified (diff)
Summary: import_statements() does not work with lazy importsFix import_statements for lazy imported objects

Changed 9 years ago by Vincent Delecroix

Attachment: trac_14767.patch added

comment:2 Changed 9 years ago by Vincent Delecroix

I strongly disagree by putting lazy as True for lazy imported objects. The behavior is nevertheless corrected with a two line patch which needs review (but the description of the ticket needs to be modified accordingly if you agree with my solution).

Vincent

comment:3 Changed 9 years ago by Vincent Delecroix

Description: modified (diff)
Status: newneeds_review

comment:4 in reply to:  2 ; Changed 9 years ago by Nicolas M. Thiéry

Replying to vdelecroix:

I strongly disagree by putting lazy as True for lazy imported objects.

What's your rationale? If something is lazy imported in the global name space, then it's likely that whoever needs it will want to lazy import it, no?

The only drawback is that lazy imported classes are a bit more fragile due to the isinstance issue.

The behavior is nevertheless corrected with a two line patch which needs review (but the description of the ticket needs to be modified accordingly if you agree with my solution).

I'll have a look.

Cheers,

Nicolas

comment:5 Changed 9 years ago by Nicolas M. Thiéry

Assuming all test pass, positive review on the current patch for resolving properly lazy imports.

comment:6 Changed 9 years ago by Nicolas M. Thiéry

Authors: Vincent Delecroix
Description: modified (diff)
Reviewers: Nicolas M. Thiéry
Status: needs_reviewneeds_info
Work issues: Do we want the bonus feature

comment:7 in reply to:  4 ; Changed 9 years ago by Vincent Delecroix

Replying to nthiery:

Replying to vdelecroix:

I strongly disagree by putting lazy as True for lazy imported objects.

What's your rationale? If something is lazy imported in the global name space, then it's likely that whoever needs it will want to lazy import it, no?

The only drawback is that lazy imported classes are a bit more fragile due to the isinstance issue.

I am not an expert of the lazy import buisness, but I do not want an extra layer on the objects I import. If I use a standard import statement inside a module it will neither affect the global namespace nor the starting time, right ?

The behavior is nevertheless corrected with a two line patch which needs review (but the description of the ticket needs to be modified accordingly if you agree with my solution).

I'll have a look.

Many thanks!

Cheers, Vincent

comment:8 in reply to:  7 Changed 9 years ago by Nicolas M. Thiéry

Replying to vdelecroix:

I am not an expert of the lazy import buisness, but I do not want an extra layer on the objects I import. If I use a standard import statement inside a module it will neither affect the global namespace nor the starting time, right ?

Assuming your module is lazy imported, yes.

So your point is that, if a module is lazy imported (which should progressively become the case most of the time), then within it the modules it depends upon may as well be imported rather than lazy imported. Ok, this sounds fair enough!

So positive review assuming the patchbot goes green.

comment:9 Changed 9 years ago by Nicolas M. Thiéry

Status: needs_infoneeds_review

comment:10 Changed 9 years ago by Nicolas M. Thiéry

Status: needs_reviewpositive_review
Work issues: Do we want the bonus feature

comment:11 Changed 9 years ago by Nicolas M. Thiéry

Description: modified (diff)

comment:12 Changed 9 years ago by Vincent Delecroix

Thanks Nicolas!

comment:13 Changed 9 years ago by Thierry Monteil

The patch works for me as well, and fixes the bug i mentionned.

However, i think both points of view are valid:

  • it is useless to add a layer when one imports a module for a direct use.
  • on the other hand, developpers using this to modify Sage's source code should know that the module is currently only lazy imported. Indeed, if they "hard" import the module, this will lose the benefits of related lazy imports in terms of startup time.

As a limited developper, if i didn't fall on the bug when writing #10358, i would have written from sage.rings.semirings.non_negative_integer_semiring import NN instead of lazy_import('sage.rings.semirings.non_negative_integer_semiring','NN').

Hence, what i may suggest it to keep Vincent's behaviour as first output, and when the module is only lazy imported in the Sage code, then add a warning explaining this, and output the lazy statement as well.

Does this make sense ?

Thierry (late as usually, or perhaps is the workflow too fast for me ;)

comment:14 in reply to:  13 Changed 9 years ago by Nicolas M. Thiéry

Replying to tmonteil:

  • on the other hand, developpers using this to modify Sage's source code should know that the module is currently only lazy imported. Indeed, if they "hard" import the module, this will lose the benefits of related lazy imports in terms of startup time.

Maybe not an issue, since now Sage complains if a lazy imported module is resolved at startup time. Ah but this might not be the case if the module is imported through another path ...

Well, let's get this patch in and think about it for the future.

comment:15 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.11.rc0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.