Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#7514 closed enhancement (fixed)

rewrite load and attach

Reported by: was Owned by: tbd
Priority: major Milestone: sage-4.3.1
Component: misc Keywords:
Cc: slabbe, robertwb, ddrake Merged in: sage-4.3.1.alpha0
Authors: William Stein Reviewers: Mitesh Patel
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by was)

DEPENDS ON #7483

The goal of this ticket is to rewrite the load and attach commands to eliminate a lot of weirdness:

(1) they should both be usable as normal python functions, so people who find the load <...> form and attach <....> forms weird, can forget they exist. This also makes comprehensive doctesting way easier.

(2) it should be possible to do load DIR+'file.sage' say, and also constructions like

for i in range(3):
    load "setup%s.py"%i

The patches hopefully do the above and much more. They refactor all the relevant code, document it way, way better, clean it up, and unify it (there was tons of replication between the notebook and command line).

The notebook page (sagenb-7514.patch) is only needed if you use the ntoebook, by the way.

Attachments (11)

sagelib-7514.patch (31.8 KB) - added by was 10 years ago.
apply to the core sage library
sagenb-7514.patch (1.8 KB) - added by was 10 years ago.
sagelib_7514-part2.patch (738 bytes) - added by was 10 years ago.
sagelib-7514_combined.patch (31.8 KB) - added by mpatel 10 years ago.
Combined sage repo patch. Depends on 4.3.alpha1, SageNB 0.4.6 (#7625), #7483, #7482.
sagenb-7514.2.patch (2.4 KB) - added by mpatel 10 years ago.
Fixed doctest failures. Replaces previous.
sagelib-7514_combined.2.patch (32.4 KB) - added by mpatel 10 years ago.
Fixed doctest failures. Depends on Sage 4.3, #7483, #7482. Replaces previous.
sagelib-7514_combined.3.patch (59.4 KB) - added by mpatel 10 years ago.
Fix(?) loading of .spyx files. Add preparser.py to reference manual. Replaces previous.
sagenb-7514-rebase.patch (2.4 KB) - added by was 10 years ago.
rebased against sagenb-0.4.8 + trac #7483.
sagenb-7514-rebase-part2.patch (3.5 KB) - added by was 10 years ago.
trac 7514 sagenb-part2: proper tracebacks; make source code introspection of functions defined in \ the notebook finally work in the notebook; properly cleanup worksheet files when notebook server is killed.
sagenb-7514-rebase-part3.patch (2.1 KB) - added by was 10 years ago.
sagenb-7514-rebase-part3.2.patch (2.1 KB) - added by mpatel 10 years ago.
Trivial docstring tweaks. Replaces previous.

Download all attachments as: .zip

Change History (36)

Changed 10 years ago by was

apply to the core sage library

Changed 10 years ago by was

comment:1 Changed 10 years ago by was

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

comment:2 follow-up: Changed 10 years ago by slabbe

  • Cc slabbe added
  • Report Upstream set to N/A

Reading the summary of this ticket made me remember a problem I was having with the attach command : Can we attach a file already in the sage tree that we are editing? I already tried, but I stopped using it because sometimes the changes in the file were not considered and I have been stopping sage and running sage -br ever since then.

Maybe I was doing something bad or maybe this ticket solves the problem or maybe it is not possible at all to do this...??

Sébastien

comment:3 Changed 10 years ago by was

Can we attach a file already in the sage tree that we are editing?

I stopped using it because sometimes the changes in the file were not

considered and I have been stopping sage and running sage -br ever since then.

You probably don't understand what attach does. All it does is execfile the file that you attached. There are situations where this happening might be perceived as "the file were not considered". E.g., if you create an install F of a class Foo defined in a file a.py, then a.py is reloaded, the object F is *not* somehow magically modified in memory to be an instance of a the new Foo that was defined in the file a.py. That's not how Python works, and it would be weird and confusing overall if things did work that way.

Changed 10 years ago by was

comment:4 Changed 10 years ago by was

  • Cc robertwb added

comment:5 in reply to: ↑ 2 Changed 10 years ago by was

  • Status changed from needs_review to needs_work

Replying to slabbe:

Reading the summary of this ticket made me remember a problem I was having with the attach command : Can we attach a file already in the sage tree that we are editing? I already tried, but I stopped using it because sometimes the changes in the file were not considered and I have been stopping sage and running sage -br ever since then.

Maybe I was doing something bad or maybe this ticket solves the problem or maybe it is not possible at all to do this...??

You just have to *understand* what attach does. It reloads the file via execfile into the global namespace when the file changes. You can attach any file, in the tree or not. But imagine this: In the sage tree there is a file foo.py. There is another file bar.py that does "import foo" and uses some code in foo. If you attach foo.py (which results in exec'ing it in the global interpreter namespaces), then that will have no impact at all on bar.py.

Thus for some problems attach works very nicely for library code, and for others it doesn't. Which is which is clearer if you know what attach does.

comment:6 Changed 10 years ago by ddrake

  • Cc ddrake added

Changed 10 years ago by mpatel

Combined sage repo patch. Depends on 4.3.alpha1, SageNB 0.4.6 (#7625), #7483, #7482.

comment:7 Changed 10 years ago by mpatel

I've attached a combined "sagelib" patch. It depends on 4.3.alpha1, SageNB 0.4.6 (#7625), #7483, and #7482.

Out of curiosity: What if we "overload" import by keeping a stack of all imported modules. When a watched file changes, we reload it and all modules loaded since the watched file was first loaded? Would this be useful in some situations?

comment:8 follow-up: Changed 10 years ago by mpatel

With 4.3.alpha1 + #7625 + #7483 + #7482 + this ticket, several doctests failed:

        sage -t  devel/sage/sage/misc/sageinspect.py # 3 doctests failed
        sage -t  devel/sage/sage/misc/preparser.py # 10 doctests failed
        sage -t  devel/sage/sage/misc/reset.pyx # 2 doctests failed
        sage -t  devel/sage/sage/misc/session.pyx # 1 doctests failed

Should we stop doctesting most (all?) of sage/server?

comment:9 in reply to: ↑ 8 Changed 10 years ago by mpatel

Replying to mpatel:

With 4.3.alpha1 + #7625 + #7483 + #7482 + this ticket, several doctests failed:

Three tests fail in sagenb.misc.sageinpect. Please see #7650.

Should we stop doctesting most (all?) of sage/server?

Please see #7534.

comment:10 Changed 10 years ago by mpatel

I noticed

sage: f = 1
sage: save(f, 'f')
sage: attach('f.sobj')
Traceback (most recent call last)
...
ValueError: argument (='f.sobj') to load or attach must have extension py, sage, or pyx                             
sage: attached_files()
['f.sobj']

Questions about loading/attaching Cython files:

  • Can they only be loaded once, if they haven't changed? For example: If zzz.pyx contains print 'Zzz!', I see
    sage: load('zzz.pyx')
    Compiling zzz.pyx...
    Zzz!
    sage: load('zzz.pyx')
    sage: load('zzz.pyx')
    sage: # Now I edit zzz.pyx
    sage: load('zzz.pyx')
    Compiling zzz.pyx...
    Awake!
    
  • Can they access existing objects? For example: If I put
    try:
        b += 1
    except:
        b = 10
    print b
    

in a .pyx file, loading the file always sets b to 10, even if it's already defined.

Disclaimer: I'm still quite ignorant about Cython.

Changed 10 years ago by mpatel

Fixed doctest failures. Replaces previous.

Changed 10 years ago by mpatel

Fixed doctest failures. Depends on Sage 4.3, #7483, #7482. Replaces previous.

comment:11 Changed 10 years ago by mpatel

  • Reviewers set to Mitesh Patel
  • Status changed from needs_work to needs_review

The latest patches should fix the doctests and the attached_files problem mentioned above. I changed attach t to attach(t), since the doctesting framework appears to use a Python interpreter, i.e., not IPython, to run the examples. I also weakened [somewhat] the tests for preparser.modified_attached_files. Is there an IPython doctesting mode?

I'm not sure about the Cython file reload problem. Should we set use_cache=True when loading .pyx files explicitly?

On globals: I see that a Cython file foo.pyx is compiled to a module and loaded via from foo import *.

Can someone else please help to review this ticket?

Changed 10 years ago by mpatel

Fix(?) loading of .spyx files. Add preparser.py to reference manual. Replaces previous.

comment:12 Changed 10 years ago by was

  • Description modified (diff)

Changed 10 years ago by was

rebased against sagenb-0.4.8 + trac #7483.

Changed 10 years ago by was

trac 7514 sagenb-part2: proper tracebacks; make source code introspection of functions defined in \ the notebook finally work in the notebook; properly cleanup worksheet files when notebook server is killed.

comment:13 Changed 10 years ago by was

Right now the files that one must apply are:

To Sage Library:
 sagelib-7514_combined.3.patch 

To Notebook:
 sagenb-7514-rebase.patch  
 sagenb-7514-rebase-part2.patch

comment:14 Changed 10 years ago by was

The file sagenb-7514-rebase-part2.patch fixes some serious issues. In particular, it makes it so we get proper tracebacks on errors, which was broken without this patch by the new architecture (of doing everything in the worksheet process, etc.). Also, source code introspection now works again for functions defined in the notebook, which many users wanted. Finally, there was a little bug where pressing control-c to stop the notebook server didn't result in quitting all worksheet processes, hence they left junk around.

comment:15 Changed 10 years ago by mpatel

  • Authors set to William Stein
  • When I load/attach a .(s)pyx file in the notebook, ___code__.py appears in the cell's output HTML.
  • Minor: Should we make attached_files() in the notebook report, e.g.,
    [DATA+'foo.py']
    
    instead of
    ['/full/path/to/foo.py']
    
  • Typing f??<TAB> for a function f defined in the notebook shows its docstring but not its source code. Is this related to ?? not working for Cython functions defined in cells or attached DATA files?
  • Should we make it possible to edit attached .pyx files, just as we can edit .py, .sage, .spyx, and .txt files?
  • Minor: save_notebook appears to be called twice on shutdown. Is this a fail-safe behavior?

comment:16 follow-up: Changed 10 years ago by was

  • Status changed from needs_review to needs_work

When I load/attach a .(s)pyx file in the notebook, _code.py appears in the cell's output HTML.

That's a bug. I'll fix it soon.

Minor: Should we make attached_files() in the notebook report

I'm not sure. But I think this should be a separate ticket, since it would be a new feature.

  • Typing f??<TAB> for a function f defined in the notebook shows its docstring but > not its source code.

That's really weird, because it is one of the problems that this patch definitely fixes. Are you sure? Did you define a function f in one input cell, then do f?? in another, and it didn't show the source code? Were you using a clean sagenb-0.4.8 install for testing? Maybe I messed up posting the patch.

Minor: save_notebook appears to be called twice on shutdown. Is this a fail-safe behavior?

This has been the case forever. It is not caused by this patch. I don't think it is desirable. Please do open a ticket.

OK, so I'm going to fix the _code_.py issue you reported above, triple check that f?? works, and if not, figure out what went wrong, then mark this as "needs review" again.

comment:17 in reply to: ↑ 16 Changed 10 years ago by mpatel

Replying to was:

  • Typing f??<TAB> for a function f defined in the notebook shows its docstring but > not its source code.

That's really weird, because it is one of the problems that this patch definitely fixes. Are you sure? Did you define a function f in one input cell, then do f?? in another, and it didn't show the source code? Were you using a clean sagenb-0.4.8 install for testing? Maybe I messed up posting the patch.

It seems that just the last line of code is omitted. The source displayed is the preparsed source, but I don't know if this is a problem.

comment:18 Changed 10 years ago by was

  • Status changed from needs_work to needs_review

The attached patch (sagenb-7514-rebase-part3.patch) should:

  1. Fixed this: "When I load/attach a .(s)pyx file in the notebook, _code.py appears in the cell's output HTML."
  1. Fixed this: "Typing f??<TAB> for a function f defined in the notebook shows its docstring but not its source code." Actually, this wasn't exactly broken... the last line of the docstring was missing. I've added a newline to the end of the input file, and now the problem is gone. This isn't a hack, since I think files are *supposed* to end with a newline, according to "IEEE Standard 1003.1 (aka POSIX)". Note that this patch *only* adds support for f?? for functions specifically entered in input normal cells (not cython, not load/attached). That's another issue.

Changed 10 years ago by was

Changed 10 years ago by mpatel

Trivial docstring tweaks. Replaces previous.

comment:19 follow-up: Changed 10 years ago by mpatel

Positive review.

If it's OK, I'll fix (e.g., INPUT/OUTPUT blocks) a few docstrings of functions affected by the sagelib patch.

comment:20 in reply to: ↑ 19 Changed 10 years ago by mpatel

  • Status changed from needs_review to positive_review

Replying to mpatel:

Positive review.

If it's OK, I'll fix (e.g., INPUT/OUTPUT blocks) a few docstrings of functions affected by the sagelib patch.

I'll make a separate ticket instead. We should include load and attach in the reference manual.

comment:21 Changed 10 years ago by timdumol

The preparsing function introduced in #7483 lacks a "# -*- coding: utf-8 -*-" header that prevents the notebook from evaluating any string that contains unicode. This is addressed in #7835.

comment:22 Changed 10 years ago by mhansen

I've merged sagelib-7514_combined.patch in sage-4.3.1.alpha0.

comment:23 Changed 10 years ago by was

  • Resolution set to fixed
  • Status changed from positive_review to closed

I merged the sagenb patches into sagenb-0.4.8.

comment:24 Changed 10 years ago by kcrisman

  • Merged in set to sage-4.3.1.alpha0

comment:25 Changed 10 years ago by mvngu

The attachment sagelib-7514_combined.3.patch in the Sage library.

Note: See TracTickets for help on using tickets.