Opened 6 years ago
Closed 6 years ago
#18908 closed defect (fixed)
Fix math-readline script
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | scripts | Keywords: | |
Cc: | ncohen, etn40ff, dunfield, slabbe | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Nathann Cohen, Sébastien Labbé, Salvatore Stella, Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | a3c75ab (Commits, GitHub, GitLab) | Commit: | a3c75ab48e7a0ef0b5f5c38cf769b58e0be35ba4 |
Dependencies: | Stopgaps: |
Description (last modified by )
sage: mathematica(1)
hangs indefinitely if Mathematica
is not installed.
The reason is that internally the script $SAGE_LOCAL/bin/math-readline
is called by Expect()
, and the latter hangs. This script should check whether the math
command has finished: currently the math-readline
script keeps running even after the subprocess has exited.
Change History (40)
comment:1 Changed 6 years ago by
- Summary changed from mathematica interface hands if mathematica is not installed to mathematica interface hangs if mathematica is not installed
comment:2 Changed 6 years ago by
- Branch set to public/18908
- Cc etn40ff dunfield slabbe added
- Commit set to 6b9a1158e64480fe9daff56e179dc9d0fcbf689d
comment:3 follow-up: ↓ 4 Changed 6 years ago by
Works for me, though I don't have mathematica either. Can someone who has it run the optional mathematica doctests with it?
Nathann
comment:4 in reply to: ↑ 3 Changed 6 years ago by
Replying to ncohen:
Works for me, though I don't have mathematica either. Can someone who has it run the optional mathematica doctests with it?
works for me with Mathematica 9, I don't seem to have Mathematica 10 though. (well, doctests fail due to numerical noise etc, but the patch itself looks good)
comment:5 follow-up: ↓ 8 Changed 6 years ago by
I'll try to fix doctests for Mathematica 9 then...
comment:6 Changed 6 years ago by
They may get pretty annoying when #18904 will be merged ;-)
comment:7 Changed 6 years ago by
basically a lot of output is texified, or semi-texified, for a reason I don't get. Typically I see things like
File "src/sage/interfaces/mathematica.py", line 44, in sage.interfaces.mathematica Failed example: mathematica('Factor[x^2-1]') # optional - mathematica Expected: (-1 + x)*(1 + x) Got: (x-1) (x+1) ********************************************************************** File "src/sage/interfaces/mathematica.py", line 46, in sage.interfaces.mathematica Failed example: mathematica('Range[3]') # optional - mathematica Expected: {1, 2, 3} Got: \{1,2,3\}
Now, the hard question: where does one have the place to describe the (pseudo)package type, i.e. optional vs experimental?!
comment:8 in reply to: ↑ 5 ; follow-up: ↓ 10 Changed 6 years ago by
comment:9 Changed 6 years ago by
I have mathematica 10.1 but the machine is currently out of service. It should be back, hopefully, late next week; I can run the doctest then.
comment:10 in reply to: ↑ 8 Changed 6 years ago by
- Status changed from new to needs_review
comment:11 follow-up: ↓ 14 Changed 6 years ago by
I have Mathematica 10.0 for Linux x86 (64-bit)
installed on my machine. I installed the branch. I did make start (I think sage -b does not update the script right?). And it seems to work:
sage: mathematica(10) 10
To make sure that the new code was really tested after the make start, I added some print:
diff --git a/src/bin/math-readline b/src/bin/math-readline index e6d40ef..ead0980 100755 --- a/src/bin/math-readline +++ b/src/bin/math-readline @@ -11,6 +11,11 @@ try: except: sys.exit('ERROR: executable math not found') +print "NEW CODE IS BEING TESTED!!!" +print "NEW CODE IS BEING TESTED!!!" +print "NEW CODE IS BEING TESTED!!!" +print "NEW CODE IS BEING TESTED!!!" + f1 = subprocess.Popen('math', shell=True, stdin=subprocess.PIPE).stdin f1.flush() try:
did make start, run sage, but then, I still get
sage: mathematica(10) 10
So is the new code really run? Is it okay if the print are not shown?
comment:12 Changed 6 years ago by
Perhaps you should run 'make' in Sage's ROOT folder before trying it again. This could copy the updated math-realine
from src/bin/
to local/bin/
.
comment:13 Changed 6 years ago by
I do see this line at the end of make start :
cp /home/labbe/Applications/sage-git/src/bin/math-readline /home/labbe/Applications/sage-git/local/bin/math-readline
comment:14 in reply to: ↑ 11 Changed 6 years ago by
Replying to slabbe:
So is the new code really run? Is it okay if the print are not shown?
I think it is, you should not get the output of math-readline within sage. It should be stripped as it is stripped the header
Mathematica 10.0 for Linux x86 (64-bit) Copyright 1988-2014 Wolfram Research, Inc.
comment:15 Changed 6 years ago by
- Reviewers set to Nathann Cohen, Sébastien Labbé, Salvatore Stella
- Status changed from needs_review to positive_review
Ok, so then, positive review!
comment:16 follow-ups: ↓ 17 ↓ 19 Changed 6 years ago by
- Status changed from positive_review to needs_info
Wait, is there a way to doctest this fix?
mathematica(10) # not mathematica Some error message...
comment:17 in reply to: ↑ 16 Changed 6 years ago by
Wait, is there a way to doctest this fix?
No, though others have asked (Dima in particular).
You should not change the status of a ticket in positive_review
to ask a question unrelated to what the ticket is supposed to address.
comment:18 Changed 6 years ago by
- Status changed from needs_info to positive_review
Ok sorry. Back to positive review.
comment:19 in reply to: ↑ 16 Changed 6 years ago by
Replying to slabbe:
Wait, is there a way to doctest this fix?
mathematica(10) # not mathematica Some error message...
like this?
diff --git a/src/sage/interfaces/mathematica.py b/src/sage/interfaces/mathematica.py index aa5a848..04eaf9f 100644 --- a/src/sage/interfaces/mathematica.py +++ b/src/sage/interfaces/mathematica.py @@ -729,6 +729,17 @@ class MathematicaElement(ExpectElement): 0 + TESTS:: + + sage: import subprocess # optional - nomathematica + sage: try: # optional - nomathematica + ....: subprocess.check_output('which math', shell=True) # optional - nomathematica + ....: except: # optional - nomathematica + ....: mathematica(10) # optional - nomathematica + Traceback (most recent call last): + ... + TypeError: ... + AUTHORS: - Felix Lawrence (2010-11-03): Major rewrite to use ._sage_repr() and
comment:20 follow-up: ↓ 32 Changed 6 years ago by
- Description modified (diff)
- Status changed from positive_review to needs_work
This is really the wrong fix.
comment:21 Changed 6 years ago by
- Description modified (diff)
comment:22 follow-up: ↓ 25 Changed 6 years ago by
I don't know what you are talking about. If I quit Sage with math available, I see
sage: Exiting Sage (CPU time 0m2.48s, Wall time 36m25.69s). Exiting Mathematica with PID 12864 running /home/scratch/dimpase/sage/sage/src/bin/math-readline
Do you mean to say that the last message is misleading?
And of course if there is no math
there then the script kills itself by sys.exit()
.
comment:23 follow-up: ↓ 28 Changed 6 years ago by
The patch on this ticket means to solve the problem of hanging if math
is not present, no more than that. And it does it. I am not aware of any other problems, leave alone ways to reproduce these other problems.
comment:24 Changed 6 years ago by
- Status changed from needs_work to positive_review
I am setting it back to positive review. Please feel free to submit a proper error report, not "wrong fix" bullsh*t, and then change the ticket status accordingly.
comment:25 in reply to: ↑ 22 Changed 6 years ago by
If I understand correctly what is being said by jdemeyer the problem is this:
$ math-readline Mathematica 10.0 for Linux x86 (64-bit) Copyright 1988-2014 Wolfram Research, Inc. In[1]:= Exit Traceback (most recent call last): File "/opt/sage/local/bin/math-readline", line 23, in <module> f1.writelines(line+'\n') IOError: [Errno 32] Broken pipe
which is quite ugly.
comment:26 Changed 6 years ago by
- Commit changed from 6b9a1158e64480fe9daff56e179dc9d0fcbf689d to a3c75ab48e7a0ef0b5f5c38cf769b58e0be35ba4
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
a3c75ab | Exit math-readline if child process exits
|
comment:27 Changed 6 years ago by
- Component changed from interfaces: optional to scripts
comment:28 in reply to: ↑ 23 Changed 6 years ago by
Sometimes it is easier to just write a patch and explain later rather than the other way around. And unfortunately there is no ticket status for "hang on, I'm working on a patch".
The real problem with the script was that, when the math
child process exited (for whatever reason, including Mathematica not being installed), the math-readline
script did not exit. The proper fix is to check for the exiting of the child process. This works no matter for which reason the child process exited (which could be calling Exit
in Mathematica, which could be Mathematica crashing, which could be Mathematica not being installed).
comment:29 Changed 6 years ago by
I also made a few more fixes to the script:
- actually using
readline
, which was the whole point of the script! - removing the empty write
sys.stdout.write('')
(what was the point of that?). - using
write()
instead ofwritelines()
to write a single string. - removing the unneeded
sys.exit()
at the end.
comment:30 Changed 6 years ago by
- Summary changed from mathematica interface hangs if mathematica is not installed to Fix math-readline script
comment:31 Changed 6 years ago by
comment:32 in reply to: ↑ 20 ; follow-up: ↓ 33 Changed 6 years ago by
Replying to jdemeyer:
This is really the wrong fix.
FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".
comment:33 in reply to: ↑ 32 ; follow-up: ↓ 34 Changed 6 years ago by
Replying to dimpase:
Replying to jdemeyer:
This is really the wrong fix.
FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".
OK, point taken. What about "This is really not the correct fix, I'm having a look at how to improve it".
comment:34 in reply to: ↑ 33 ; follow-up: ↓ 35 Changed 6 years ago by
Replying to jdemeyer:
Replying to dimpase:
Replying to jdemeyer:
This is really the wrong fix.
FYI, I found this comment very annoying. Unless you want to alienate people, always say in such cases instead "I would like to improve this".
OK, point taken. What about "This is really not the correct fix, I'm having a look at how to improve it"
s/correct/complete would do; indeed, saying that something is incorrect/wrong without giving an explanation could create a tension you don't need.
comment:35 in reply to: ↑ 34 Changed 6 years ago by
Replying to dimpase:
saying that something is incorrect/wrong without giving an explanation could create a tension you don't need.
But it was really incorrect: the fact that the script hangs when math
is not installed was a symptom of the problem, not the problem itself.
But at the time when I send that message, I didn't have a clear explanation of the real problem: I knew that the proposed solution was wrong, but I couldn't yet say what the right fix would be. Like I said: it's often easier to explain after you have a working patch.
And I had to act quickly because, these days, tickets which have positive_review are often closed very soon.
comment:36 follow-up: ↓ 37 Changed 6 years ago by
The script hanged without math
installed, and hanged Sage in return. And that few lines that were in the fix solved this particular issue, obviously an improvement. Anything that is an improvement cannot be called incorrect.
comment:37 in reply to: ↑ 36 ; follow-up: ↓ 39 Changed 6 years ago by
Replying to dimpase:
Anything that is an improvement cannot be called incorrect.
I completely disagree with this. An improvement which hides a bug is incorrect.
comment:38 Changed 6 years ago by
- Reviewers changed from Nathann Cohen, Sébastien Labbé, Salvatore Stella to Nathann Cohen, Sébastien Labbé, Salvatore Stella, Dima Pasechnik
- Status changed from needs_review to positive_review
lgtm
comment:39 in reply to: ↑ 37 Changed 6 years ago by
Replying to jdemeyer:
Replying to dimpase:
Anything that is an improvement cannot be called incorrect.
I completely disagree with this. An improvement which hides a bug is incorrect.
except that this one did not hide a bug, at least not any more than the original scrip did. Anyhow, this is not the point; the point is that you should not brand anything at all "wrong", "incorrect" without a justification. (Unless you out to annoy people, and I hope that this is not the case).
comment:40 Changed 6 years ago by
- Branch changed from public/18908 to a3c75ab48e7a0ef0b5f5c38cf769b58e0be35ba4
- Resolution set to fixed
- Status changed from positive_review to closed
moving the discussion from the closed #16703 here. I propose the patch in the attached
public/18908
branch. I don't have a system with mathematica installed, I only tested it works nicely withoutmath
present, both stand-alone and as well as producing the right result when callingmathematica(10)
from Sage.