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 )
- Cythonize for speed.
- Insert original object into namespace on resolution
- Support import *
Apply only
Attachments (6)
Change History (36)
Changed 10 years ago by
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- 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
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
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
comment:5 Changed 9 years ago by
- 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
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
- Description modified (diff)
comment:8 Changed 9 years ago by
- Cc rishi added
comment:9 Changed 9 years ago by
- Cc jason added
comment:10 Changed 9 years ago by
- 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
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
- 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
- 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: ↓ 16 Changed 9 years ago by
- 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
- Description modified (diff)
comment:16 in reply to: ↑ 14 Changed 9 years ago by
- 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
- Status changed from needs_work to needs_review
Sorry, I missed the dependency... :/
comment:18 Changed 9 years ago by
The code looks good. Is there a reason why _cmp_ does not have a test?
comment:19 Changed 9 years ago by
{{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
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
No, you're absolutely right, that (half) doctest is an oversight on my part.
Changed 9 years ago by
comment:22 Changed 9 years ago by
I've updated the patch.
comment:23 Changed 9 years ago by
- 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
- Description modified (diff)
comment:25 Changed 9 years ago by
- Status changed from needs_review to positive_review
Your documentation is correct, thanks.
comment:26 Changed 9 years ago by
- Reviewers changed from Luis Felipe Tabera to Luis Felipe Tabera Alonso
comment:27 Changed 9 years ago by
- 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
comment:28 Changed 9 years ago by
- 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
- 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
- Merged in set to sage-4.6.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
I split it up into two patches for easier refereeing. The first simply moves lazy_import.py to lazy_import.pyx.