Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#14523 closed defect (fixed)

can't exit or detach after error in attached file

Reported by: mstreng Owned by: was
Priority: major Milestone: sage-5.11
Component: user interface Keywords:
Cc: leif, nbruin, slabbe Merged in: sage-5.11.rc0
Authors: Volker Braun Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14266 Stopgaps:

Description (last modified by vbraun)

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

Attachments (3)

trac_14523_improve_attach.patch (48.4 KB) - added by vbraun 8 years ago.
Updated patch
trac_14523_readline.2.patch (16.2 KB) - added by vbraun 8 years ago.
Updated patch
trac_14523_readline.patch (16.2 KB) - added by vbraun 8 years ago.
Updated patch

Download all attachments as: .zip

Change History (45)

comment:1 Changed 8 years ago by vbraun

  • 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

comment:2 Changed 8 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

comment:3 Changed 8 years ago by hthomas

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.

comment:4 Changed 8 years ago by vbraun

Updated patch

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

comment:5 Changed 8 years ago by vbraun

  • Cc leif nbruin added

comment:6 Changed 8 years ago by hthomas

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

comment:7 Changed 8 years ago by hthomas

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

comment:8 Changed 8 years ago by vbraun

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

comment:9 Changed 8 years ago by hthomas

@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?

comment:10 follow-up: Changed 8 years ago by 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.

But thats for another ticket.

comment:11 in reply to: ↑ 10 Changed 8 years ago by mstreng

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.

comment:12 Changed 8 years ago by vbraun

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

comment:13 follow-up: Changed 8 years ago by leif

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

comment:14 in reply to: ↑ 13 Changed 8 years ago by leif

Replying to leif:

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?

comment:15 follow-up: Changed 8 years ago by vbraun

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.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by leif

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.)

comment:17 in reply to: ↑ 16 Changed 8 years ago by vbraun

Replying to leif:

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...

Last edited 8 years ago by vbraun (previous) (diff)

comment:18 Changed 8 years ago by vbraun

Fixed failing doctest on patchbot

comment:19 Changed 8 years ago by vbraun

  • Dependencies set to #14266

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

Changed 8 years ago by vbraun

Updated patch

comment:20 Changed 8 years ago by tscrim

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

comment:21 Changed 8 years ago by vbraun

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.

Last edited 8 years ago by vbraun (previous) (diff)

comment:22 Changed 8 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:23 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:24 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Doctest failures:

sage -t devel/sage/sage/misc/sageinspect.py
**********************************************************************
File "devel/sage/sage/misc/sageinspect.py", line 57, in sage.misc.sageinspect
Failed example:
    import sage.misc.attach
Exception raised:
    Traceback (most recent call last):
      File "/mazur/release/merger/sage-5.11.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 486, in _run
        self.execute(example, compiled, test.globs)
      File "/mazur/release/merger/sage-5.11.beta2/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 845, in execute
        exec compiled in globs
      File "<doctest sage.misc.sageinspect[10]>", line 1, in <module>
        import sage.misc.attach
    ImportError: No module named attach
**********************************************************************

and

sage -t --long devel/sage/sage/misc/inputhook.py
**********************************************************************
File "devel/sage/sage/misc/inputhook.py", line 42, in sage.misc.inputhook.sage_inputhook
Failed example:
    shell.run_cell('sage_inputhook()')
Expected:
    ### reloading attached file tmp_....py modified at ... ###
    0
Got:
    0
**********************************************************************
File "devel/sage/sage/misc/inputhook.py", line 46, in sage.misc.inputhook.sage_inputhook
Failed example:
    shell.run_cell('a')
Expected:
    3
Got:
    2
**********************************************************************

comment:25 follow-up: Changed 8 years ago by vbraun

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.

comment:26 in reply to: ↑ 25 Changed 8 years ago by mstreng

Replying to vbraun:

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

comment:27 Changed 8 years ago by vbraun

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...

comment:28 Changed 8 years ago by vbraun

  • Description modified (diff)

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.

comment:29 Changed 8 years ago by vbraun

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

comment:30 Changed 8 years ago by vbraun

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.

comment:31 Changed 8 years ago by vbraun

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).

comment:32 Changed 8 years ago by vbraun

comment:33 Changed 8 years ago by vbraun

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

comment:34 Changed 8 years ago by jhpalmieri

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
----------------------------------------------------------------------

Changed 8 years ago by vbraun

Updated patch

Changed 8 years ago by vbraun

Updated patch

comment:35 Changed 8 years ago by vbraun

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

patchbot: apply trac_14523_improve_attach.patch trac_14523_readline.patch

comment:36 Changed 8 years ago by slabbe

  • Cc slabbe added

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).

Last edited 8 years ago by slabbe (previous) (diff)

comment:37 Changed 8 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:38 Changed 8 years ago by tscrim

  • Status changed from needs_review to positive_review

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

comment:39 Changed 8 years ago by vbraun

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

comment:40 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.11.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 Changed 6 years ago by jhpalmieri

See #16784 for a followup.

Note: See TracTickets for help on using tickets.