Opened 10 years ago

Closed 9 years ago

#8456 closed enhancement (fixed)

lazy import improvements

Reported by: robertwb Owned by: tbd
Priority: major Milestone: sage-4.6.2
Component: misc Keywords:
Cc: rishi, jason, rlm Merged in: sage-4.6.2.alpha3
Authors: Robert Bradshaw Reviewers: Luis Felipe Tabera Alonso
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by lftabera)

  1. Cythonize for speed.
  2. Insert original object into namespace on resolution
  3. Support import *

Related: #7502, #8254.

Apply only

  1. 8456-lazy-import-cython-rebase-4.6.patch
  2. 8456-lazy-import-enhancements-4.6.patch
  3. 8456-lazy-import-documentation.patch

Attachments (6)

8456-lazy-import-cython.patch (14.3 KB) - added by robertwb 10 years ago.
8456-lazy-import-enhancements.patch (8.2 KB) - added by robertwb 10 years ago.
8456-lazy-import-fixes.patch (1.1 KB) - added by robertwb 9 years ago.
8456-lazy-import-cython-rebase-4.6.patch (14.3 KB) - added by robertwb 9 years ago.
apply instead of 8456-lazy-import-cython.patch
8456-lazy-import-enhancements-4.6.patch (21.6 KB) - added by robertwb 9 years ago.
8456-lazy-import-documentation.patch (1.4 KB) - added by robertwb 9 years ago.

Download all attachments as: .zip

Change History (36)

Changed 10 years ago by robertwb

Changed 10 years ago by robertwb

comment:1 Changed 10 years ago by robertwb

  • Status changed from new to needs_review

I split it up into two patches for easier refereeing. The first simply moves lazy_import.py to lazy_import.pyx.

comment:2 Changed 10 years ago by SimonKing

  • Status changed from needs_review to needs_work

I started with importing the first patch only. I tried to lazy-import ZZ, but it did not properly work:

sage: from sage.misc.lazy_import import lazy_import
sage: lazy_import('sage.rings.all', 'ZZ')
sage: type(ZZ)
<type 'sage.rings.integer_ring.IntegerRing_class'>

According to the doc tests, the result should be different.

I then imported the second patch. When starting sage, I got

---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/IPython/ipmaker.pyc in force_import(modname)
     64         reload(sys.modules[modname])
     65     else:
---> 66         __import__(modname)
     67
     68

/home/king/SAGE/sage-4.3.1/local/bin/ipy_profile_sage.py in <module>()
      5     preparser(True)
      6
----> 7     import sage.all_cmdline
      8     sage.all_cmdline._init_cmdline(globals())
      9

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/all_cmdline.py in <module>()
     12 try:
     13
---> 14     from sage.all import *
     15     from sage.calculus.predefined import x
     16     preparser(on=True)

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/all.py in <module>()
    323 # Cache the contents of star imports.

    324 import sage.misc.lazy_import
--> 325 sage.misc.lazy_import.save_cache_file()
    326
    327

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/misc/lazy_import.so in sage.misc.lazy_import.save_cache_file (sage/misc/lazy_import.c:1977)()

OSError: [Errno 18] Invalid cross-device link
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

No idea what this means. But I think it is "needs work".

Cheers, Simon

comment:3 Changed 10 years ago by SimonKing

PS:

When quitting sage after the unsuccessful start, I got

sage: quit
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/king/SAGE/sage-4.3.1/local/bin/<ipython console> in <module>()

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/IPython/iplib.pyc in ipmagic(self, arg_s)
    951         else:
    952             magic_args = self.var_expand(magic_args,1)
--> 953             return fn(magic_args)
    954
    955     def ipalias(self,arg_s):

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/IPython/Magic.pyc in magic_quit(self, parameter_s)
   2478         """Exit IPython, confirming if configured to do so (like %exit)"""
   2479
-> 2480         self.shell.exit()
   2481
   2482     def magic_Exit(self, parameter_s=''):

/home/king/SAGE/sage-4.3.1/local/lib/python2.6/site-packages/sage/all.py in _quit_sage_(self)
    292         self.exit_now = True
    293     if self.exit_now:
--> 294         quit_sage()
    295         self.exit_now = True
    296

TypeError: 'NoneType' object is not callable

Even worse, hg_sage does not work. So, it is more difficult now to repair my sage installation...

comment:4 Changed 10 years ago by robertwb

Hmm... is this on top of an alpha? Worked fine for me.

As for backing out, you can do "sage -hg rollback"

Changed 9 years ago by robertwb

comment:5 Changed 9 years ago by robertwb

  • Status changed from needs_work to needs_review

OK, the problem was I was using os.rename rather than, say, shutil.move, which caused errors when the DOT_SAGE file was not on the same filesystem as temp files. Glad you caught that, ready for another review.

comment:6 Changed 9 years ago by mpatel

  • Authors set to Robert Bradshaw

Simon, could you look at this again, when it's convenient? I can try to review this, but I'm not sure that I'm sufficiently familiar with Cython.

For me, all three patches apply cleanly to 4.5.3.alpha1 (using a Mercurial queue).

comment:7 Changed 9 years ago by mpatel

  • Description modified (diff)

comment:8 Changed 9 years ago by rishi

  • Cc rishi added

comment:9 Changed 9 years ago by jason

  • Cc jason added

comment:10 Changed 9 years ago by robertwb

  • Cc rlm added

Lots of people want to see a faster Sage startup; is anyone willing to review this?

comment:11 Changed 9 years ago by lftabera

rebase 8456-lazy-import-cython.patch to sage 4.6

patches apply and doctest pass. The code looks ok, but I do not know (yet) cython nor python introspection to give a positive review.

comment:12 Changed 9 years ago by lftabera

  • Status changed from needs_review to needs_info

As a user, I would be puzzled if I found at the begining of the session:

sage: InfinityRing
<sage.misc.lazy_import.LazyImport object at 0x7f51dca0e290>
sage: InfinityRing()
Zero
sage: InfinityRing
The Infinity Ring

So I think that LazyImport? should have a repr method that inserts the real object in the namespace.

The problem is that once you start with this kind of things you do not know where to stop...

sage: infinity in InfinityRing
TypeError: argument of type 'sage.misc.lazy_import.LazyImport' is not iterable

So, what about repr, contains, _latex_, cmp ? Is it worth to add them to the class? Should this be in another ticket?

comment:13 Changed 9 years ago by robertwb

  • Status changed from needs_info to needs_work

Hmm... it looks like I need to manually add the typeslot methods, as they don't get forwarded with getattr. Thanks for catching this, I'll post another patch.

comment:14 follow-up: Changed 9 years ago by robertwb

  • Status changed from needs_work to needs_review

Apply only 8456-lazy-import-cython-rebase-4.6.patch and 8456-lazy-import-enhancements-4.6.patch. This fixes the issue with __repr__ and all the other special methods.

comment:15 Changed 9 years ago by rlm

  • Description modified (diff)

comment:16 in reply to: ↑ 14 Changed 9 years ago by rlm

  • Status changed from needs_review to needs_work

Replying to robertwb:

Apply only 8456-lazy-import-cython-rebase-4.6.patch and 8456-lazy-import-enhancements-4.6.patch. This fixes the issue with __repr__ and all the other special methods.

Are you sure that's right?

rlmill@fermat:~/sage-4.6.1.alpha2/devel/sage-main$ hg qpush
applying 8456-lazy-import-cython-rebase-4.6.patch
unable to find 'sage/misc/lazy_import.pyx' for patching
8 out of 8 hunks FAILED -- saving rejects to file sage/misc/lazy_import.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 8456-lazy-import-cython-rebase-4.6.patch

comment:17 Changed 9 years ago by rlm

  • Status changed from needs_work to needs_review

Sorry, I missed the dependency... :/

Changed 9 years ago by robertwb

apply instead of 8456-lazy-import-cython.patch

comment:18 Changed 9 years ago by lftabera

The code looks good. Is there a reason why _cmp_ does not have a test?

comment:19 Changed 9 years ago by robertwb

{{cmp}} does have a test.

The buildbot needs to know to apply 8456-lazy-import-cython-rebase-4.6.patch and 8456-lazy-import-enhancements-4.6.patch

comment:20 Changed 9 years ago by lftabera

I may be misunderstanding, since ther is also a richcmp method and cmp method is not used, but I see the following code:

    def __cmp__(left, right):
        """
        TESTS::
        
            sage: lazy_import('sage.all', 'ZZ'); lazy_ZZ = ZZ
        """
        return binop(cmp, left, right)

comment:21 Changed 9 years ago by robertwb

No, you're absolutely right, that (half) doctest is an oversight on my part.

Changed 9 years ago by robertwb

comment:22 Changed 9 years ago by robertwb

I've updated the patch.

comment:23 Changed 9 years ago by lftabera

  • Milestone changed from sage-4.6.1 to sage-4.6.2
  • Reviewers set to Luis Felipe Tabera

The code looks right so I give it a positive review.

However, I upload a patch to add some minor documentation explaining the parameters of lazy_import. Robert, could you please review and check that I did not make a mistake?

comment:24 Changed 9 years ago by lftabera

  • Description modified (diff)

comment:25 Changed 9 years ago by robertwb

  • Status changed from needs_review to positive_review

Your documentation is correct, thanks.

comment:26 Changed 9 years ago by lftabera

  • Reviewers changed from Luis Felipe Tabera to Luis Felipe Tabera Alonso

comment:27 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Please check Sphinx syntax

docstring of sage.misc.lazy_import.get_star_imports:1: (WARNING/2) Inline emphasis start-string without end-string.

Changed 9 years ago by robertwb

comment:28 Changed 9 years ago by robertwb

  • Status changed from needs_work to needs_review

I've refreshed the documentation patch to escape this asterisk.

comment:29 Changed 9 years ago by robertwb

  • Status changed from needs_review to positive_review

I'm re-instating the positive review, as adding the asterisk escape was a superficial change.

comment:30 Changed 9 years ago by jdemeyer

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