Opened 6 years ago

Closed 6 years ago

#14767 closed defect (fixed)

Fix import_statements for lazy imported objects

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

Description (last modified by nthiery)

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 vdelecroix 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by nthiery

  • Cc vdelecroix added
  • Description modified (diff)
  • Summary changed from import_statements() does not work with lazy imports to Fix import_statements for lazy imported objects

Changed 6 years ago by vdelecroix

comment:2 follow-up: Changed 6 years ago by vdelecroix

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 6 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by 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.

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 6 years ago by nthiery

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

comment:6 Changed 6 years ago by nthiery

  • Authors set to Vincent Delecroix
  • Description modified (diff)
  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_review to needs_info
  • Work issues set to Do we want the bonus feature

comment:7 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by vdelecroix

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 6 years ago by nthiery

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 6 years ago by nthiery

  • Status changed from needs_info to needs_review

comment:10 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review
  • Work issues Do we want the bonus feature deleted

comment:11 Changed 6 years ago by nthiery

  • Description modified (diff)

comment:12 Changed 6 years ago by vdelecroix

Thanks Nicolas!

comment:13 follow-up: Changed 6 years ago by tmonteil

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 6 years ago by nthiery

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 6 years ago by jdemeyer

  • Merged in set to sage-5.11.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.