#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 )
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)
Change History (45)
comment:1 Changed 9 years ago by
- 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 9 years ago by
- Status changed from new to needs_review
comment:3 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
- Cc leif nbruin added
comment:6 Changed 9 years ago by
Thanks very much! (I have tested attaching a file with an error.)
comment:7 Changed 9 years ago by
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 9 years ago by
Updated patch fixes some minor things, makes imports lazy, and adds doctests.
comment:9 Changed 9 years ago by
@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: ↓ 11 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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: ↓ 14 Changed 9 years ago by
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 9 years ago by
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: ↓ 16 Changed 9 years ago by
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: ↓ 17 Changed 9 years ago by
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 9 years ago by
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
...
comment:18 Changed 9 years ago by
Fixed failing doctest on patchbot
comment:19 Changed 9 years ago by
- Dependencies set to #14266
Conflicts with the otherwise unrelated #14266, this is now a dependency.
comment:20 Changed 9 years ago by
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 9 years ago by
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.
comment:22 Changed 9 years ago by
- 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 9 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:24 Changed 9 years ago by
- 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: ↓ 26 Changed 9 years ago by
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 9 years ago by
comment:27 Changed 9 years ago by
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 9 years ago by
- 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 9 years ago by
I also added an interface to readline so we can redraw the prompt.
comment:30 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Notebook issue is here: https://github.com/sagemath/sagenb/issues/169
comment:33 Changed 9 years ago by
Patch rebased for sage-5.11.beta1 (not yet released)
comment:34 Changed 9 years ago by
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 ----------------------------------------------------------------------
comment:35 Changed 9 years ago by
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 9 years ago by
- Cc slabbe added
comment:37 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:38 Changed 9 years ago by
- 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 9 years ago by
Thanks! I believe the patchbot uses separate branches so it doesn't have to sync-build.
comment:40 Changed 9 years ago by
- Merged in set to sage-5.11.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:41 Changed 9 years ago by
comment:42 Changed 8 years ago by
See #16784 for a followup.