can't exit or detach after error in attached file

[mstreng]

17:57:03:~$ touch myfile.sage
17:57:05:~$ sage
----------------------------------------------------------------------
| Sage Version 5.8, Release Date: 2013-03-15                         |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
sage: %attach myfile.sage
sage: file('myfile.sage', 'w').write('1/0')
sage: 1+1
...
ZeroDivisionError: Rational division by zero
sage: exit
...
ZeroDivisionError: Rational division by zero
sage: detach('myfile.sage')
...
ZeroDivisionError: Rational division by zero

It seems like the only way to continue using this Sage session is to fix the error first. But suppose you have some reason not to (e.g. are in a hurry, and the error is hard to fix). I think it would be much better if Sage was to evaluate "1+1" and especially "exit" and "detach" even when unable to reload one of the attached files.

Possible ways to fix this are discussed on sage-devel.

Apply

• attachment: trac_14523_improve_attach.patch
• attachment: trac_14523_readline.patch

Depends on #14266

CC: @nexttime @nbruin @seblabbe

Component: user interface

Author: Volker Braun

Reviewer: Travis Scrimshaw

Merged: sage-5.11.rc0

Issue created by migration from https://trac.sagemath.org/ticket/14523

[vbraun]

comment:1

• only reload once per time stamp change
• reload without having to press any key in Sage
• collect all attach-related files in one module instead of sprinkling it around

[vbraun]

Author: Volker Braun

[hughrthomas]

comment:3

I'm not knowledgeable enough to offer a useful review of the code. It does seem like a good solution to the observed problem.

There is a behaviour of "attach" which bugs me. If the file you try to attach generates an error when you attach it, then it doesn't actually get attached. I don't see the point of this. It makes even less sense now than before. Would it be easy to fix on this ticket, or should I open another one?

I do also have one comment about the current patch -- if it wouldn't be too complicated, I think it would be nice to get a new sage prompt after the reloading is finished, to indicate that it has completed.

[vbraun]

comment:4

Updated patch

• remember that files is attached even if load fails
• don't reload files that are in the process of being written

[hughrthomas]

comment:6

Thanks very much! (I have tested attaching a file with an error.)

[hughrthomas]

comment:7

Bonus: this also seems to fix the issue reported at #14149: when files are attached, spurious files are created in the current directory.

[vbraun]

comment:8

Updated patch fixes some minor things, makes imports lazy, and adds doctests.

[hughrthomas]

comment:9

@Volker: Thanks for the explanation (at #14149), and the new doctests, which clarify what is happening with the creation of files. Would it be possible to create the temp files somewhere else? (I guess there are security concerns with putting them in /tmp, but isn't there any alternative?) Or, would it be possible to keep track of them and remove them? I guess at each update, the previous copy could be deleted, and detach or quit could delete the otherwise-remaining copy?

[vbraun]

comment:10

I think the "debug mode" for load and attach should be removed. Files should always be preparsed in memory. We should keep a user-accessible list of loaded/attached files with some state, preparsed form, etc. Any debugging that you can do with temporary files can be done much nicer by querying the files on the command line.

But thats for another ticket.

[mstreng]

comment:11

Replying to @vbraun:

I think the "debug mode" for load and attach should be removed. Files should always be preparsed in memory. We should keep a user-accessible list of loaded/attached files with some state, preparsed form, etc. Any debugging that you can do with temporary files can be done much nicer by querying the files on the command line.

I don't understand the last sentence, and (so?) I completely disagree with the rest. Tracebacks for attached files should be completely detailed, with code snippets. Anything that has to be done by hand for each of the steps in the traceback is far from as useful as immediately seeing the code itself. See #11812 and this discussion.

[vbraun]

comment:12

The updated patch

• changes the default back to "attach debug mode" (really execute-via-file mode)
• moves the temp file to Sage's temporary directory
• adds doctests that the source snippet is shown

[nexttime]

comment:13

Is SAGE_LOAD_ATTACH_PATH cleared for doctesting? Otherwise some doctests may fail (if the user has actually set it).

[nexttime]

comment:14

Replying to @nexttime:

Is SAGE_LOAD_ATTACH_PATH cleared for doctesting? Otherwise some doctests may fail (if the user has actually set it).

Just checked in vanilla Sage 5.10.beta1, and there indeed three doctests fail (in sage/misc/preparser.py) if it is set to something non-empty.

So apparently this is not new, but could you also fix it here?

[vbraun]

comment:15

I think the undocumented SAGE_LOAD_ATTACH_PATH should be removed, not groomed further. There should be a Python-level way to set the search path if desired. If you really need the functionality then you just have to call sage.path(os.environ['SAGE_LOAD_ATTACH_PATH']) in your own code. But that should be done on another ticket.

[nexttime]

comment:16

Replying to @vbraun:

I think the undocumented SAGE_LOAD_ATTACH_PATH should be removed, not groomed further.

I have no strong opinion on doing so. Although it's apparently not documented (at least not in Sage's documentation besides the docstring; it may get mentioned in some 3rd party documentation), I bet a couple of people will complain (probably much) later if we remove it... ;-)

But I'd rather tend to leave the (IMHO convenient) functionality, probably documenting it more prominently (it doesn't really fit into the Sage Installation Guide, just like other environment variables, which is a different story), and fixing potential doctest failures (which is a minor issue anyway) -- elsewhere, on another ticket.

There should be a Python-level way to set the search path if desired. If you really need the functionality then you just have to call sage.path(os.environ['SAGE_LOAD_ATTACH_PATH']) in your own code. But that should be done on another ticket.

So you don't intend to remove it here either? (Ok for me, just asking.)

[vbraun]

comment:17

Replying to @nexttime:

So you don't intend to remove it here either? (Ok for me, just asking.)

No, I'd rather leave that for later. The whole load/attach functionality should be collected into an object instead of multiple global functions, including the search path functionality. Then we can go ahead and deprecate SAGE_LOAD_ATTACH_PATH...

[vbraun]

comment:18

Fixed failing doctest on patchbot

[vbraun]

Dependencies: #14266

[vbraun]

comment:19

Conflicts with the otherwise unrelated #14266, this is now a dependency.

[vbraun]

Updated patch

[tscrim]

comment:20

Attachment: trac_14523_improve_attach.patch.gz

Hey Volker,

Looks good for the most part. It's somewhat minor, but is there some way we can get it so that we get it to end with the sage prompt? For me it currently ends at a blank line, although it still accepts input correctly.

Thanks,

Travis

[vbraun]

comment:21

Its not just print a new prompt, you want to redraw the current line (since you could have a partially-written command there). I think its possible to call rl_refresh_line in readline, but that would need some careful testing so we don't break other stuff.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:22

True. I'm happy with getting this into Sage as is.

[vbraun]

comment:25

Another issue: computations from attach'ed files are uninterruptable. This is because readline installs its own signal handlers while it is sitting at the input prompt, and (by default) only restores the previous handler when you press enter.

[mstreng]

comment:26

Replying to @vbraun:

Could this cause other problems with CTRL-C as well? See #14740:6

[vbraun]

comment:27

A closer reading of IPython reveals that it intentionally overwrites the SIGINT handler when you set an inputhook (as we do to poll for file changes). So interrupting computations is completely messed up, even if its not attached. I'm working on a new patch...

[vbraun]

comment:28

All subsequent changes will be in the separate trac_14523_readline.patch. This does now sidesteps IPython to preserve our signal handlers. Also, it really needs to be implemented in Cython since the inputhook must be able to ignore signals so it does not get interrupted immediately when you press Ctrl-C at the prompt.

[vbraun]

comment:29

I also added an interface to readline so we can redraw the prompt.

[vbraun]

comment:30

Also fixed Jeroen's doctest failures. First is stale python file (you need to run sage -sync-build to reproduce it), second is filesystem timestamp granularity.

[vbraun]

comment:31

Slightly beautified so the old command line is erased:

sage: attach('foo.py')
output from attached file
sage: 123_<-- cursor here 456

Now edit and save attached file, and you get

sage: attach('foo.py')
output from attached file
### reloading attached file t.py modified at 18:04:08 ###
output from changed attached file
sage: 123_<-- cursor here 456

On a related note, attaching files in the notebook is completely broken (with and without this ticket).

[vbraun]

comment:32

Notebook issue is here: sagemath/sagenb#169

[vbraun]

comment:33

Patch rebased for sage-5.11.beta1 (not yet released)

[jhpalmieri]

comment:34

I'm getting doctest failures:

sage -t devel/sage/sage/misc/sage_extension.py
**********************************************************************
File "devel/sage/sage/misc/sage_extension.py", line 107, in sage.misc.sage_extension.SageMagics.attach
Failed example:
    shell.run_cell('sage_inputhook()')
Expected:
    ### reloading attached file run_cell.py modified at ... ###
    0
Got:
    0
**********************************************************************
File "devel/sage/sage/misc/sage_extension.py", line 111, in sage.misc.sage_extension.SageMagics.attach
Failed example:
    shell.run_cell('a')
Expected:
    3
Got:
    2
**********************************************************************
1 item had failures:
   2 of  15 in sage.misc.sage_extension.SageMagics.attach
    [45 tests, 2 failures, 2.13 s]
----------------------------------------------------------------------
sage -t devel/sage/sage/misc/sage_extension.py  # 2 doctests failed
----------------------------------------------------------------------

[vbraun]

Attachment: trac_14523_readline.2.patch.gz

Updated patch

[vbraun]

Attachment: trac_14523_readline.patch.gz

Updated patch

[vbraun]

comment:35

Thanks, fixed. Was another instance of filesytem timestamp granularity in the doctest.

patchbot:
apply trac_14523_improve_attach.patch trac_14523_readline.patch

[seblabbe]

comment:36

For a reference, #14149 and #14169 were closed as duplicate of this ticket (they are not obvious duplicate just looking at the titles or descriptions).

[tscrim]

comment:38

Hey Volker,

Sorry for letting this sit for so long. Looks good to me (I remembered to run sync-build this time to remove the old files before testing).

(Side note, does the patchbot run sync-build after applying patches? If not, I think doing this would be beneficial.)

Best,

Travis

[vbraun]

comment:39

Thanks! I believe the patchbot uses separate branches so it doesn't have to sync-build.

[jdemeyer]

Merged: sage-5.11.rc0

[jhpalmieri]

comment:41

See http://ask.sagemath.org/question/3084/notebook-and-initsage and #15308.

[jhpalmieri]

comment:42

See #16784 for a followup.