Sage: Ticket #10296: Singular interface wasting time by waiting for the prompt too often
https://trac.sagemath.org/ticket/10296
<p>
There are two fundamental differences between the Singular and the Gap interfaces:
</p>
<ol><li>The Singular interface uses a synchronisation method in each call to its eval method. Gap doesn't.
</li><li>The Gap interface deletes variables by declaring that their name may be overwritten. This is not possible in Singular, since Singular variables are statically typed. Thus, the Singular interface explicitly kills the underlying variable in Singular, which requires one call to _eval_line for each variable that is to be deleted.
</li></ol><p>
Unfortunately, waiting for the prompt to appear in a pseudo terminal may be very costly on some systems, since select.select() may be very slow - see <a class="new ticket" href="https://trac.sagemath.org/ticket/10294" title="defect: Slowness of pexpect interfaces on some machines (new)">#10294</a>. So, additional calls to _eval_line should be strictly avoided!
</p>
<p>
The first point makes me wonder: Why has the synchronisation method become necessary for Singular but not for Gap?
</p>
<p>
The second point makes me wonder whether it is really necessary to use one _eval_line for each single deletion.
</p>
<p>
Moreover, I wonder whether the two points are actually related: I could imagine that synchronisation became necessary when garbage collection, accidentally performed inside an _eval_line, and thus sending an _eval_line inside an outer _eval_line, caused a dead lock in the outer _eval_line. Was that the case, historically?
</p>
<p>
Suggestions:
</p>
<ol><li><code>singular.eval(cmd)</code> is currently simply passing <code>cmd</code> to _eval_line, after synchronisation and after killing unused variables. I suggest to add the code for killing unused variables to <code>cmd</code>, thus using _eval_line only once.
</li><li>If my above guess on the role of synchronisation is correct, then one could simply drop synchronisation after implementing my first suggestion.
</li></ol><p>
This is related with <a class="new ticket" href="https://trac.sagemath.org/ticket/10294" title="defect: Slowness of pexpect interfaces on some machines (new)">#10294</a> and perhaps with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10295" title="enhancement: Upgrade and optimize pexpect (closed: fixed)">#10295</a>. Cc to William and Willem-Jan, since they wrote the synchronisation code.
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
<p>
For the release manager:
</p>
<p>
Apply
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10296/trac10296_singular_overhead_combined.patch" title="Attachment 'trac10296_singular_overhead_combined.patch' in Ticket #10296">trac10296_singular_overhead_combined.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10296/trac10296_singular_overhead_combined.patch" title="Download"></a>
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10296
Trac 1.1.6SimonKingSat, 20 Nov 2010 12:35:28 GMT
https://trac.sagemath.org/ticket/10296#comment:1
https://trac.sagemath.org/ticket/10296#comment:1
<p>
Committing the kills in <code>singular.eval</code> by prepending stuff to the given command turned out to break a lot of doc tests, since they test against the given command showing up in the output. But I think one could be even closer to the Gap interface.
</p>
<p>
Recall that in the Gap interface, variables are freed by allowing to re-use their names. So, memory allocated in Gap is only freed when the old variable is actually overwritten.
</p>
<p>
In Singular, memory is freed by the command <code>kill</code>. I suggest to use this command not inside <code>singular.eval</code> but inside <code>SingularElement.__init__</code>: It is put in front of the command that creates the new variable. Then, the analogy between Gap and Singular interfaces is rather close: Memory in Gap or in Singular is freed only when a new Element is created -- by overwriting the old stuff in the case of Gap, or by killing the old stuff in the case of Singular.
</p>
<p>
Moreover, I removed the synchronisation code, since it seems to me that synchronisation is not really necessary if the garbage collection is not using an additional _eval_line (which is the case with my suggestion).
</p>
<p>
So, there remains only <em>one</em> call to select.select when creating a new Singular element, rather than two plus the number of old variables to be killed.
</p>
<p>
Indeed, my approach reduces the overhead in using the Singular interface by 2/3 in the test above!
</p>
<p>
But there occur two doctest failures, that I have to deal with now.
</p>
TicketAlexanderDreyerMon, 22 Nov 2010 08:40:25 GMTcc changed
https://trac.sagemath.org/ticket/10296#comment:2
https://trac.sagemath.org/ticket/10296#comment:2
<ul>
<li><strong>cc</strong>
<em>AlexanderDreyer</em> added
</li>
</ul>
TicketSimonKingTue, 23 Nov 2010 12:34:31 GMTstatus, summary changed; author set
https://trac.sagemath.org/ticket/10296#comment:3
https://trac.sagemath.org/ticket/10296#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Simon King</em>
</li>
<li><strong>summary</strong>
changed from <em>Singular interface wasting time by calling select.select() too often</em> to <em>Singular interface wasting time by waiting for the prompt too often</em>
</li>
</ul>
<p>
My patch does the following.
</p>
<p>
<strong><span class="underline">Garbage Collection</span></strong>
</p>
<p>
In the Singular interface, freeing the memory used for old variables in Singular requires to send a "kill" command to Singular, presumably by _eval_line.
</p>
<p>
A problem would result if garbage collection would create such _eval_line inside an outer _eval_line: The inner _eval_line would cause the outer _eval_line to hang forever. I guess this was the reason why the del method does not actually delete the variables but only marks them for deletion; the actual deletion is postponed to the next use of "eval".
</p>
<p>
I guess that for the same reason, synchronisation is done at the beginning of "eval". <strong>Please correct me, if the reason for synchronising so frequently is different'''
</strong></p>
<p>
I suggest to do the deletions a bit less frequently, namely in the "set" method rather than in the "eval" method. Moreover, I suggest to send the kill command not by a separate _eval_line (one for each old variable), but to prepend the join of all kill commands to the command that creates the new variable.
</p>
<p>
Since killing does not require an additional _eval_line, nesting can not occur. So, synchronisation is not needed.
</p>
<p>
<strong><span class="underline">Overhead Reduction</span></strong>
</p>
<p>
As mentioned in the ticket description, calling _eval_line frequently is bad, since waiting for the Singular prompt in the output of a pseudo terminal produces a massive overhead (up to 22ms per call) on some machines.
</p>
<p>
My patch removes synchronisation (which avoids one call to _eval_line per call to eval), and killing old variables does not require any additional _eval_line. So, in the example proposed in the ticket description, the wall time drops by 2/3!
</p>
<p>
<strong><span class="underline">Miscellaneous</span></strong>
</p>
<p>
Synchronisation used to fail with an <code>AttributeError</code> for the GAP interface. It is fixed by the patch.
</p>
<p>
I tried to make everything more stable, by giving more opportunities to detect that the interface crashed, and restarting if necessary.
</p>
<p>
Of course, I added documentation and tests that (I think) cover all issues mentioned here. Moreover, I added tests covering some optional arguments to _eval_line.
</p>
<p>
For me, <code>sage -testall</code> passes. Hence, ready for review!
</p>
TicketSimonKingWed, 29 Dec 2010 16:36:42 GMT
https://trac.sagemath.org/ticket/10296#comment:4
https://trac.sagemath.org/ticket/10296#comment:4
<p>
The old patch needed to be rebased.
</p>
<p>
The new patch version should apply on top of <code>sage-4.6.1.alpha3</code>. I am now running doctests, but I'll keep it "needs review" for now.
</p>
TicketSimonKingWed, 29 Dec 2010 17:21:01 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:5
https://trac.sagemath.org/ticket/10296#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Not good. There are several timeouts in the tests. I need to work on it.
</p>
TicketSimonKingWed, 29 Dec 2010 17:55:30 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:6
https://trac.sagemath.org/ticket/10296#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:5" title="Comment 5">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Not good. There are several timeouts in the tests.
</p>
</blockquote>
<p>
It seems that I forgot <code>sage -b</code>. I repeated the tests that previously had a timeout, and now it is fine.
</p>
<p>
I put it back to "needs review".
</p>
TicketSimonKingTue, 18 Jan 2011 07:18:14 GMT
https://trac.sagemath.org/ticket/10296#comment:7
https://trac.sagemath.org/ticket/10296#comment:7
<p>
Since the nagbot currently does not seem to send messages: I hope you don't mind that I'm sending a reminder about speeding up the Singular pexpect interface.
</p>
TicketSimonKingFri, 11 Feb 2011 07:58:36 GMT
https://trac.sagemath.org/ticket/10296#comment:8
https://trac.sagemath.org/ticket/10296#comment:8
<p>
I am sorry to nag you again - could someone take a look at the patch? The patchbot has no complaints at that point.
</p>
TicketSimonKingTue, 08 Mar 2011 15:15:51 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/10296#comment:9
https://trac.sagemath.org/ticket/10296#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>fix tests</em>
</li>
</ul>
<p>
Too late...
</p>
<p>
Meanwhile the patchbot finds some errors. I try to find the reason.
</p>
TicketSimonKingTue, 08 Mar 2011 16:05:53 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/10296#comment:10
https://trac.sagemath.org/ticket/10296#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>fix tests</em> deleted
</li>
</ul>
<p>
Apparently, a <code>StdOutContext</code> was recently introduced, and the expected output for one doctest has changed by my patch. I changed the test slightly, so that now it should work.
</p>
TicketSimonKingSat, 09 Apr 2011 07:08:13 GMTcc changed
https://trac.sagemath.org/ticket/10296#comment:11
https://trac.sagemath.org/ticket/10296#comment:11
<ul>
<li><strong>cc</strong>
<em>malb</em> added
</li>
</ul>
<p>
I think reducing the overhead in the Singular pexpect interface by 2/3 is a good thing. But the ticket is starting to become old.
</p>
<p>
Review, anyone?
</p>
TicketmalbSat, 09 Apr 2011 15:46:23 GMTstatus, type changed; reviewer set
https://trac.sagemath.org/ticket/10296#comment:12
https://trac.sagemath.org/ticket/10296#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Martin Albrecht</em>
</li>
<li><strong>type</strong>
changed from <em>defect</em> to <em>enhancement</em>
</li>
</ul>
<ul><li>the patch applies cleanly against 4.7.alpha3
</li><li>long doctests pass
</li><li>the patch is well documented
</li><li>the patch does look okay
</li></ul><p>
So all good. The only caveat: I cannot claim I thought long and hard about the possible implications of changing this stuff. But unless someone comes up with a good example why Simon's approach is wrong, I'd say positive review.
</p>
TicketjdemeyerTue, 12 Apr 2011 13:46:17 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/10296#comment:13
https://trac.sagemath.org/ticket/10296#comment:13
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.7</em> to <em>sage-4.7.1</em>
</li>
</ul>
<p>
This will need to be rebased because it conflicts with <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
TicketSimonKingTue, 12 Apr 2011 14:40:33 GMT
https://trac.sagemath.org/ticket/10296#comment:14
https://trac.sagemath.org/ticket/10296#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:13" title="Comment 13">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
This will need to be rebased because it conflicts with <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
</blockquote>
<p>
Really unfortunate. Apparently, in order to apply <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> to my sage-4.6.2, I also need to import further patches.
</p>
TicketjdemeyerTue, 12 Apr 2011 15:22:17 GMT
https://trac.sagemath.org/ticket/10296#comment:15
https://trac.sagemath.org/ticket/10296#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:14" title="Comment 14">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:13" title="Comment 13">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
This will need to be rebased because it conflicts with <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
</blockquote>
<p>
Really unfortunate. Apparently, in order to apply <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> to my sage-4.6.2, I also need to import further patches.
</p>
</blockquote>
<p>
The easiest solution is probably to download <a class="ext-link" href="http://boxen.math.washington.edu/home/release/sage-4.7.alpha5/sage-4.7.alpha5.tar"><span class="icon"></span>http://boxen.math.washington.edu/home/release/sage-4.7.alpha5/sage-4.7.alpha5.tar</a> which includes <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>. This version of Sage is not released and will certainly change and might have doctest failures but at least it gives you a target to rebase to.
</p>
TicketSimonKingWed, 13 Apr 2011 08:38:00 GMTwork_issues set
https://trac.sagemath.org/ticket/10296#comment:16
https://trac.sagemath.org/ticket/10296#comment:16
<ul>
<li><strong>work_issues</strong>
set to <em>make gap restart after quitting or crashing</em>
</li>
</ul>
<p>
It is really <em>really</em> unfortunate. The changes to the gap interface (so that gap restarts after quitting, instead of throwing an error) are now not working any more.
</p>
TicketSimonKingWed, 13 Apr 2011 09:38:05 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/10296#comment:17
https://trac.sagemath.org/ticket/10296#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>make gap restart after quitting or crashing</em> deleted
</li>
</ul>
<p>
I updated my patch. It should now apply to sage-4.7.alpha5. The long tests in <code>sage/interfaces/</code> all pass.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:16" title="Comment 16">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
It is really <em>really</em> unfortunate. The changes to the gap interface (so that gap restarts after quitting, instead of throwing an error) are now not working any more.
</p>
</blockquote>
<p>
The reason for that was a typo when I applied one of the hunks manually.
</p>
<p>
Note that I changed <code>.interrupt()</code> into <code>.interrupt(timeout=1)</code> in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.
</p>
<p>
Does that suffice to return to the positive review?
</p>
<p>
In any case, I am now running all long doctests.
</p>
TicketSimonKingWed, 13 Apr 2011 11:26:55 GMT
https://trac.sagemath.org/ticket/10296#comment:18
https://trac.sagemath.org/ticket/10296#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:17" title="Comment 17">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Note that I changed <code>.interrupt()</code> into <code>.interrupt(timeout=1)</code> in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.
</p>
<p>
Does that suffice to return to the positive review?
</p>
<p>
In any case, I am now running all long doctests.
</p>
</blockquote>
<p>
FWIW: The long doctests all pass.
</p>
TicketmalbWed, 13 Apr 2011 11:50:49 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:19
https://trac.sagemath.org/ticket/10296#comment:19
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
yes, back to positive review.
</p>
TicketwjpWed, 13 Apr 2011 12:01:18 GMT
https://trac.sagemath.org/ticket/10296#comment:20
https://trac.sagemath.org/ticket/10296#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:17" title="Comment 17">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Note that I changed <code>.interrupt()</code> into <code>.interrupt(timeout=1)</code> in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.
</p>
</blockquote>
<p>
I haven't looked at the patch in detail, but unexplained behaviour related to synchronization/interrupts in a patch modifying synchronization sounds a bit scary, and a reason to think about it some more...
</p>
TicketSimonKingWed, 13 Apr 2011 12:20:15 GMT
https://trac.sagemath.org/ticket/10296#comment:21
https://trac.sagemath.org/ticket/10296#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:20" title="Comment 20">wjp</a>:
</p>
<blockquote class="citation">
<p>
I haven't looked at the patch in detail, but unexplained behaviour related to synchronization/interrupts in a patch modifying synchronization sounds a bit scary, and a reason to think about it some more...
</p>
</blockquote>
<p>
For the record: The test in question is new.
</p>
<p>
In sage-4.6 (as I just tested on sage.math), the test would in fact not work at all: When you do
</p>
<pre class="wiki">sage: cutoff = singular._eval_using_file_cutoff
sage: singular._eval_using_file_cutoff = 4
sage: singular._eval_line('for(int i=1;i<=3;i++){i=1;};', wait_for_prompt=False)
</pre><p>
then you needed to interrupt it manually.
</p>
<p>
Now, the problem is that after the lines above, <code>sage: singular.interrupt()</code> is supposed to return <code>False</code>, which it always did on the command line, but in some (not all) doctest runs returns <code>True</code>.
</p>
<p>
Since we wouldn't have been able to test it without my patch, we actually don't know
</p>
<blockquote>
<p>
(1) whether the instability existed before and was uncovered by the patch,
(2) whether the problem was introduced by my patch, or
(3) whether the problem was a result of the changes to the pexpect interface in <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
</blockquote>
<p>
The latter could actually be the case: Today was the first time ever that the <code>singular.interrupt()</code> example was problematic - and it was the first time that I worked with the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
TicketjdemeyerWed, 13 Apr 2011 19:45:28 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10296#comment:22
https://trac.sagemath.org/ticket/10296#comment:22
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.1.alpha0</em>
</li>
</ul>
TicketjdemeyerThu, 14 Apr 2011 18:25:10 GMTdescription changed
https://trac.sagemath.org/ticket/10296#comment:23
https://trac.sagemath.org/ticket/10296#comment:23
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10296?action=diff&version=23">diff</a>)
</li>
</ul>
TicketjdemeyerThu, 14 Apr 2011 18:30:41 GMT
https://trac.sagemath.org/ticket/10296#comment:24
https://trac.sagemath.org/ticket/10296#comment:24
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:21" title="Comment 21">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
The latter could actually be the case: Today was the first time ever that the <code>singular.interrupt()</code> example was problematic - and it was the first time that I worked with the patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
</blockquote>
<p>
I have decided <em>not</em> to merge <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> in the sage-4.7 cycle. I will merge it in sage-4.7.1.alpha0, such that potential issues with <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> can be sorted out.
</p>
TicketSimonKingThu, 14 Apr 2011 18:38:43 GMT
https://trac.sagemath.org/ticket/10296#comment:25
https://trac.sagemath.org/ticket/10296#comment:25
<p>
Shouldn't this ticket then re-opened as well?
</p>
<p>
And should I perhaps provide two patch versions here, one depending on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> and one independent?
</p>
TicketjdemeyerThu, 14 Apr 2011 18:47:56 GMT
https://trac.sagemath.org/ticket/10296#comment:26
https://trac.sagemath.org/ticket/10296#comment:26
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:25" title="Comment 25">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Shouldn't this ticket then re-opened as well?
</p>
<p>
And should I perhaps provide two patch versions here, one depending on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> and one independent?
</p>
</blockquote>
<p>
For the moment, the plan is still to merge <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> in sage-4.7.1.alpha0 together with <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a> and a fixed <a class="closed ticket" href="https://trac.sagemath.org/ticket/10818" title="defect: EclLib should allow signals to make LISP code interruptable (closed: fixed)">#10818</a>. So for the moment, I think we can leave this ticket as it is.
</p>
TicketjpfloriFri, 15 Apr 2011 11:13:57 GMT
https://trac.sagemath.org/ticket/10296#comment:27
https://trac.sagemath.org/ticket/10296#comment:27
<p>
I've ran more than 100 tests of expect.py and gap.py without "timeout=1" (so using the default value of timeout=0.3) on Sage 4.7.alpha3 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/10818" title="defect: EclLib should allow signals to make LISP code interruptable (closed: fixed)">#10818</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a>, built from scratch on debian unstable/experimental amd64, intel core 2 quad, and everything passed.
</p>
<p>
By the way, hg failed to apply the patch, I had to manually apply the following hunk:
</p>
<pre class="wiki">--- gap.py
+++ gap.py
@@ -475,46 +532,45 @@
</pre><p>
The only difference is that the tests are obviously longer with a greater value of timeout.
</p>
<p>
Could anyone else check whether the default value of timeout gives a random behavior, at least as often as every two times?
</p>
TicketSimonKingFri, 15 Apr 2011 12:01:09 GMT
https://trac.sagemath.org/ticket/10296#comment:28
https://trac.sagemath.org/ticket/10296#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:27" title="Comment 27">jpflori</a>:
</p>
<blockquote class="citation">
<p>
I've ran more than 100 tests of expect.py and gap.py without "timeout=1" (so using the default value of timeout=0.3) on Sage 4.7.alpha3 + <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/10818" title="defect: EclLib should allow signals to make LISP code interruptable (closed: fixed)">#10818</a> + <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a>, built from scratch on debian unstable/experimental amd64, intel core 2 quad, and everything passed.
</p>
</blockquote>
<p>
Quite a relief...
</p>
<p>
I have Debian GNU/Linux squeeze/sid, and I think our administrator did patch the kernel in order to make pseudo-Tty faster. The reason is that, without that kernel patch, the overhead of all pexpect interfaces is abundant. It is, of course, imaginable that the kernel patch is responsible for the flaky behaviour of the test on my machine.
</p>
TicketjpfloriFri, 15 Apr 2011 13:53:47 GMT
https://trac.sagemath.org/ticket/10296#comment:29
https://trac.sagemath.org/ticket/10296#comment:29
<p>
Ok, I've now done 500 tests of both without problems.
</p>
TicketjdemeyerTue, 03 May 2011 12:27:27 GMTstatus changed; resolution, merged deleted
https://trac.sagemath.org/ticket/10296#comment:30
https://trac.sagemath.org/ticket/10296#comment:30
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
<li><strong>merged</strong>
<em>sage-4.7.1.alpha0</em> deleted
</li>
</ul>
<p>
I get many failures on the buildbot, mainly on non-Linux systems (e.g. OS X, Solaris). The failures look like the following:
</p>
<pre class="wiki">sage -t -long -force_lib devel/sage/sage/interfaces/expect.py
**********************************************************************
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/interfaces/expect.py", line 702:
sage: singular._eval_line_using_file('def a=3;')
Exception raised:
Traceback (most recent call last):
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_15[5]>", line 1, in <module>
singular._eval_line_using_file('def a=3;')###line 702:
sage: singular._eval_line_using_file('def a=3;')
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 731, in _eval_line_using_file
s = self._eval_line(self._read_in_file_command(tmp_to_use), allow_use_file=False)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 841, in _eval_line
raise RuntimeError, "%s\nError evaluating %s in %s"%(msg, line, self)
RuntimeError: [Errno 5] Input/output error
Error evaluating < "/tmp/dot_sage.gjyZgjfltL/temp/bsd.local/8324//interface//tmp8502"; in Singular
**********************************************************************
[..many more similar errors..]
sage -t -long -force_lib devel/sage/sage/interfaces/gap.py
**********************************************************************
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/interfaces/gap.py", line 521:
sage: a = gap(3)
Exception raised:
Traceback (most recent call last):
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_9[10]>", line 1, in <module>
a = gap(Integer(3))###line 521:
sage: a = gap(3)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 201, in __call__
return self._coerce_from_special_method(x)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 227, in _coerce_from_special_method
return (x.__getattribute__(s))(self)
File "sage_object.pyx", line 463, in sage.structure.sage_object.SageObject._gap_ (sage/structure/sage_object.c:3988)
File "sage_object.pyx", line 439, in sage.structure.sage_object.SageObject._interface_ (sage/structure/sage_object.c:3628)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 199, in __call__
return cls(self, x, name=name)
File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 1286, in __init__
raise TypeError, x
TypeError: Error evaluating $sage9:=3;; in Gap
**********************************************************************
</pre>
TicketSimonKingTue, 03 May 2011 14:17:00 GMT
https://trac.sagemath.org/ticket/10296#comment:31
https://trac.sagemath.org/ticket/10296#comment:31
<p>
That's bad. Perhaps its a difference in new line / carriage return characters?
</p>
<p>
Probably I need to build sage on bsd.math before being able to test.
</p>
TicketjdemeyerTue, 03 May 2011 16:29:39 GMT
https://trac.sagemath.org/ticket/10296#comment:32
https://trac.sagemath.org/ticket/10296#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:31" title="Comment 31">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
That's bad. Perhaps its a difference in new line / carriage return characters?
</p>
</blockquote>
<p>
<code>RuntimeError: [Errno 5] Input/output error</code> seems to indicate something more low-level related to the OS (for example, the handling of pseudo-tty's).
</p>
<blockquote class="citation">
<p>
Probably I need to build sage on bsd.math before being able to test.
</p>
</blockquote>
<p>
You can start working from the non-released sage-4.7.1.alpha0: <a class="ext-link" href="http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar"><span class="icon"></span>http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar</a>. This currently includes <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> but not <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a>, but this is of course subject to change.
</p>
TicketSimonKingTue, 03 May 2011 16:39:02 GMT
https://trac.sagemath.org/ticket/10296#comment:33
https://trac.sagemath.org/ticket/10296#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:32" title="Comment 32">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
You can start working from the non-released sage-4.7.1.alpha0: <a class="ext-link" href="http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar"><span class="icon"></span>http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar</a>. This currently includes <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a> but not <a class="closed ticket" href="https://trac.sagemath.org/ticket/10296" title="enhancement: Singular interface wasting time by waiting for the prompt too often (closed: fixed)">#10296</a>, but this is of course subject to change.
</p>
</blockquote>
<p>
Thank you, but the compilation of sage-4.7 on bsd.math is already in the final stages (building documentation, if I am not mistaken).
</p>
TicketSimonKingTue, 03 May 2011 20:41:50 GMT
https://trac.sagemath.org/ticket/10296#comment:34
https://trac.sagemath.org/ticket/10296#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:32" title="Comment 32">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
<code>RuntimeError: [Errno 5] Input/output error</code> seems to indicate something more low-level related to the OS (for example, the handling of pseudo-tty's).
</p>
</blockquote>
<p>
That could be. I already found: If you quit gap by sending "quit;" through the gap interface (by gap.eval) and then try to evaluate a command, then you get a different error or error message on bsd.math than on my computer.
</p>
<p>
What my patch did was to search for "EOL" in the error message, which should be fast and suffices on my computer (Linux). If it does not suffice (as on bsd.math) then one could still test gap._expect.isalive(). That's slower, but apparently more robust.
</p>
<p>
The other error you are reporting seems similar in spirit, but is in a different place.
</p>
TicketSimonKingWed, 04 May 2011 07:17:26 GMT
https://trac.sagemath.org/ticket/10296#comment:35
https://trac.sagemath.org/ticket/10296#comment:35
<p>
I just submitted a new patch that solves part of the problem: There are different error types raised on linux and non-linux machines if a pexpect interfaces crashes. But there are some doctest failures left, so, it still needs work.
</p>
TicketSimonKingWed, 04 May 2011 07:29:40 GMT
https://trac.sagemath.org/ticket/10296#comment:36
https://trac.sagemath.org/ticket/10296#comment:36
<p>
The problem with the remaining doctest failures is that my patch introduces a new argument <code>first_call</code> to <code>sage.interfaces.expect.Expect._eval_line</code>: The idea is to restart the interface if it is crashed unexpectedly and then re-eval the line, but only once, to avoid infinite loops. The <code>first_call</code> argument tells whether it is the first evaluation, or the re-evaluation after a crash.
</p>
<p>
But some interfaces override <code>_eval_line</code> and don't know about the new argument. It should thus be added.
</p>
TicketSimonKingWed, 04 May 2011 08:42:15 GMT
https://trac.sagemath.org/ticket/10296#comment:37
https://trac.sagemath.org/ticket/10296#comment:37
<p>
Fortunately I can do some optional tests (e.g. maple) on bsd.math. It revealed another problem: Some code relies on a specific error message. But in my patch, that error is caught and replaced by a generic error telling that the interface crashed. That should be taken care of as well.
</p>
TicketSimonKingWed, 04 May 2011 09:10:11 GMTattachment set
https://trac.sagemath.org/ticket/10296
https://trac.sagemath.org/ticket/10296
<ul>
<li><strong>attachment</strong>
set to <em>trac10296_expect_on_nonlinux.patch</em>
</li>
</ul>
<p>
Make the automatic restart of pexpect interfaces work on non-linux machines
</p>
TicketSimonKingWed, 04 May 2011 09:12:33 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:38
https://trac.sagemath.org/ticket/10296#comment:38
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
I think I got it solved: The long doctests in sage/interfaces pass both on my machine and on bsd.math. This is based on sage-4.7.
</p>
<p>
In addition, the optional tests of sage/interfaces/maple.py pass on bsd.math as well, with the exception of those that fail with pure sage-4.7 anyway (see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11288" title="defect: Maple optional tests (closed: duplicate)">#11288</a>).
</p>
<p>
Depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/7377" title="enhancement: Symbolic Ring to Maxima via EclObject (closed: fixed)">#7377</a>.
</p>
TicketSimonKingWed, 04 May 2011 12:03:03 GMT
https://trac.sagemath.org/ticket/10296#comment:39
https://trac.sagemath.org/ticket/10296#comment:39
<p>
PS: Since the new patch is not totally trivial, I think someone should look at it, i.e., not simply restoring the old positive review.
</p>
TicketjdemeyerWed, 25 May 2011 13:47:29 GMT
https://trac.sagemath.org/ticket/10296#comment:40
https://trac.sagemath.org/ticket/10296#comment:40
<p>
*bump*
</p>
<p>
Any reviewers?
</p>
TicketfbisseyWed, 25 May 2011 18:36:21 GMT
https://trac.sagemath.org/ticket/10296#comment:41
https://trac.sagemath.org/ticket/10296#comment:41
<p>
Actually from the description of the first patch it could help with some problems in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9958" title="enhancement: Upgrade python to 2.7.x (closed: fixed)">#9958</a> so I will take a look and test them on linux and OS X.
</p>
TicketfbisseyFri, 27 May 2011 04:41:41 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:42
https://trac.sagemath.org/ticket/10296#comment:42
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
</p>
<pre class="wiki">sage -t -long "devel/sage/sage/interfaces/gap.py"
**********************************************************************
File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514:
sage: gap.interrupt(timeout=1)
Expected:
True
Got:
False
**********************************************************************
</pre><p>
This is a test introduced in <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10296/trac10296_singular_overhead.patch" title="Attachment 'trac10296_singular_overhead.patch' in Ticket #10296">trac10296_singular_overhead.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10296/trac10296_singular_overhead.patch" title="Download"></a>
</p>
TicketSimonKingFri, 27 May 2011 05:42:43 GMT
https://trac.sagemath.org/ticket/10296#comment:43
https://trac.sagemath.org/ticket/10296#comment:43
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:42" title="Comment 42">fbissey</a>:
</p>
<blockquote class="citation">
<p>
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
</p>
<pre class="wiki">sage -t -long "devel/sage/sage/interfaces/gap.py"
**********************************************************************
File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514:
sage: gap.interrupt(timeout=1)
Expected:
True
Got:
False
**********************************************************************
</pre></blockquote>
<p>
That test is really annoying. I tried to make it more reliable by adding the timeout parameter, but apparently it does not work like that.
</p>
<p>
However, the main point is that without the patch, we had
</p>
<pre class="wiki">sage: cutoff = gap._eval_using_file_cutoff
sage: gap._eval_using_file_cutoff = 4
sage: gap._eval_line('while(1=1) do i:=1;; od;', wait_for_prompt=False)
<Runs forever>
</pre><p>
In other words, we would not even come to the line <code>gap.interrupt()</code>.
</p>
<p>
I would not mind if the new test simply made sure that no error is raised. It does not matter, IMHO, whether we have the return value <code>True</code> or <code>False</code>. So, perhaps change it into
</p>
<pre class="wiki">sage: gap.interrupt(timeout=1) is not None
True
</pre><p>
or
</p>
<pre class="wiki">sage: gap.interrupt(timeout=1)
...
</pre><p>
?
</p>
TicketfbisseyMon, 30 May 2011 03:32:01 GMT
https://trac.sagemath.org/ticket/10296#comment:44
https://trac.sagemath.org/ticket/10296#comment:44
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:43" title="Comment 43">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:42" title="Comment 42">fbissey</a>:
</p>
<blockquote class="citation">
<p>
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
</p>
<pre class="wiki">sage -t -long "devel/sage/sage/interfaces/gap.py"
**********************************************************************
File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514:
sage: gap.interrupt(timeout=1)
Expected:
True
Got:
False
**********************************************************************
</pre></blockquote>
<p>
That test is really annoying. I tried to make it more reliable by adding the timeout parameter, but apparently it does not work like that.
</p>
<p>
However, the main point is that without the patch, we had
</p>
<pre class="wiki">sage: cutoff = gap._eval_using_file_cutoff
sage: gap._eval_using_file_cutoff = 4
sage: gap._eval_line('while(1=1) do i:=1;; od;', wait_for_prompt=False)
<Runs forever>
</pre><p>
In other words, we would not even come to the line <code>gap.interrupt()</code>.
</p>
<p>
I would not mind if the new test simply made sure that no error is raised. It does not matter, IMHO, whether we have the return value <code>True</code> or <code>False</code>. So, perhaps change it into
</p>
<pre class="wiki">sage: gap.interrupt(timeout=1) is not None
True
</pre><p>
or
</p>
<pre class="wiki">sage: gap.interrupt(timeout=1)
...
</pre><p>
?
</p>
</blockquote>
<p>
The later one would imply that the output is random, but we would miss it if an error was raised so I guess the former would be better.
</p>
<pre class="wiki">gap.interrupt(timeout=1) is not None
</pre><p>
has my vote.
</p>
TicketleifFri, 22 Jul 2011 06:36:49 GMTcc changed
https://trac.sagemath.org/ticket/10296#comment:45
https://trac.sagemath.org/ticket/10296#comment:45
<ul>
<li><strong>cc</strong>
<em>leif</em> added
</li>
</ul>
<p>
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing <code>testlong</code> rather than <code>ptestlong</code>...
</p>
<p>
Can someone (finish? and) review this, or what's stopping it?
</p>
TicketSimonKingFri, 22 Jul 2011 08:45:46 GMT
https://trac.sagemath.org/ticket/10296#comment:46
https://trac.sagemath.org/ticket/10296#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:45" title="Comment 45">leif</a>:
</p>
<blockquote class="citation">
<p>
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing <code>testlong</code> rather than <code>ptestlong</code>...
</p>
<p>
Can someone (finish? and) review this, or what's stopping it?
</p>
</blockquote>
<p>
I think the problem was that a couple of weeks ago I had other things on my mind, and thus I forgot to do the change into <code>gap.interrupt(timeout=1) is not None</code>. I guess I'll do it now.
</p>
TicketleifFri, 22 Jul 2011 08:56:25 GMT
https://trac.sagemath.org/ticket/10296#comment:47
https://trac.sagemath.org/ticket/10296#comment:47
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:46" title="Comment 46">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:45" title="Comment 45">leif</a>:
</p>
<blockquote class="citation">
<p>
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing <code>testlong</code> rather than <code>ptestlong</code>...
</p>
</blockquote>
<p>
I think the problem was that a couple of weeks ago I had other things on my mind, and thus I forgot to do the change into <code>gap.interrupt(timeout=1) is not None</code>. I guess I'll do it now.
</p>
</blockquote>
<p>
That would be nice. Some tests (e.g. <code>sandpile</code>) here take more than 20 minutes each when running <code>make testlong</code>, which is totally odd.
</p>
TicketSimonKingFri, 22 Jul 2011 10:13:09 GMT
https://trac.sagemath.org/ticket/10296#comment:48
https://trac.sagemath.org/ticket/10296#comment:48
<p>
Also, the first patch doesn't apply to sage-4.7.rc2. So, I'll take care of that as well.
</p>
<p>
Concerning idleness: It seems that there is a certain kernel patch for Ubuntu that improves the pexpect performance. Namely, the problem with some OS like Ubuntu is that the latency of pseudo-tty is to much (if I understood correctly). After our sysadmin applied that patch, the problem vanished.
</p>
<p>
So, the main problem with pexpect on some platforms is not a Sage problem. That said, reducing the traffic in the pexpect interface is a good idea anyway...
</p>
TicketSimonKingFri, 22 Jul 2011 10:22:55 GMTstatus, description changed
https://trac.sagemath.org/ticket/10296#comment:49
https://trac.sagemath.org/ticket/10296#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10296?action=diff&version=49">diff</a>)
</li>
</ul>
<p>
I have updated the first patch, which should now apply against sage-4.7.1.rc2. I have changed the doctest in question, so that now it is only tested that interrupting gap has worked.
</p>
<p>
For the patchbot:
</p>
<p>
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch
</p>
TicketSimonKingMon, 01 Aug 2011 15:57:50 GMTattachment set
https://trac.sagemath.org/ticket/10296
https://trac.sagemath.org/ticket/10296
<ul>
<li><strong>attachment</strong>
set to <em>trac10296_singular_overhead.patch</em>
</li>
</ul>
<p>
Modify garbage collection in Singular interface, which reduces the overhead. Synchronization of Gap interface.
</p>
TicketSimonKingMon, 01 Aug 2011 15:59:33 GMT
https://trac.sagemath.org/ticket/10296#comment:50
https://trac.sagemath.org/ticket/10296#comment:50
<p>
I had to rebase the first patch against sage-4.7.1.rc1. I had a typo in my previous post: The old patch was relative sage-4.7.rc2, not 4.7.1.rc2.
</p>
<p>
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch
</p>
<p>
By the way: It still needs review...
</p>
TicketSimonKingTue, 30 Aug 2011 10:22:11 GMT
https://trac.sagemath.org/ticket/10296#comment:51
https://trac.sagemath.org/ticket/10296#comment:51
<p>
Any reviewer for a patch that improves the performance of the Singular pexpect interface?
</p>
TicketmalbTue, 30 Aug 2011 12:41:21 GMT
https://trac.sagemath.org/ticket/10296#comment:52
https://trac.sagemath.org/ticket/10296#comment:52
<p>
patchbot claims tests failed but the full log shows all tests passed. Not sure what happened there. I'm running ptestlong on sage.math now.
</p>
TicketmalbTue, 30 Aug 2011 13:52:51 GMT
https://trac.sagemath.org/ticket/10296#comment:53
https://trac.sagemath.org/ticket/10296#comment:53
<p>
I re-read the patches and I still think they are good, so I'm close to a positive review. One - perhaps nitpicking - point though: shouldn't <code>first_call</code> be <code>restart_if_needed</code> or something? "First call" isn't very telling what it does?
</p>
TicketSimonKingTue, 30 Aug 2011 14:12:37 GMT
https://trac.sagemath.org/ticket/10296#comment:54
https://trac.sagemath.org/ticket/10296#comment:54
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:53" title="Comment 53">malb</a>:
</p>
<blockquote class="citation">
<p>
I re-read the patches and I still think they are good, so I'm close to a positive review. One - perhaps nitpicking - point though: shouldn't <code>first_call</code> be <code>restart_if_needed</code> or something? "First call" isn't very telling what it does?
</p>
</blockquote>
<p>
That sounds like a good idea.
</p>
TicketmhansenSun, 18 Dec 2011 14:41:34 GMTattachment set
https://trac.sagemath.org/ticket/10296
https://trac.sagemath.org/ticket/10296
<ul>
<li><strong>attachment</strong>
set to <em>trac_10296-restart_if_needed.patch</em>
</li>
</ul>
TicketmhansenSun, 18 Dec 2011 14:41:55 GMT
https://trac.sagemath.org/ticket/10296#comment:55
https://trac.sagemath.org/ticket/10296#comment:55
<p>
I've added a patch which makes this change. Let's get this in.
</p>
TicketSimonKingFri, 02 Mar 2012 09:52:20 GMTdescription changed
https://trac.sagemath.org/ticket/10296#comment:56
https://trac.sagemath.org/ticket/10296#comment:56
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10296?action=diff&version=56">diff</a>)
</li>
</ul>
<p>
Martin, can you look whether my patches together with Mike's improvement of the wording is fine?
</p>
<p>
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch trac_10296-restart_if_needed.patch
</p>
TicketmalbFri, 02 Mar 2012 17:31:40 GMT
https://trac.sagemath.org/ticket/10296#comment:57
https://trac.sagemath.org/ticket/10296#comment:57
<p>
Hi,
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10296/trac_10296-restart_if_needed.patch" title="Attachment 'trac_10296-restart_if_needed.patch' in Ticket #10296">trac_10296-restart_if_needed.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10296/trac_10296-restart_if_needed.patch" title="Download"></a> has no commit message
</li><li>I get lots of errors when applying <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10296/trac10296_expect_on_nonlinux.patch" title="Attachment 'trac10296_expect_on_nonlinux.patch' in Ticket #10296">trac10296_expect_on_nonlinux.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10296/trac10296_expect_on_nonlinux.patch" title="Download"></a> to 5.0-beta6?
</li></ul>
TicketSimonKingFri, 02 Mar 2012 18:10:11 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/10296#comment:58
https://trac.sagemath.org/ticket/10296#comment:58
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>rebase</em>
</li>
</ul>
<p>
Bad. Needs work, then...
</p>
<p>
Concerning the missing commit message: I think I'll produce a combined patch with a commit message.
</p>
TicketSimonKingSat, 03 Mar 2012 07:52:09 GMTdependencies set
https://trac.sagemath.org/ticket/10296#comment:59
https://trac.sagemath.org/ticket/10296#comment:59
<ul>
<li><strong>dependencies</strong>
set to <em>#11431</em>
</li>
</ul>
<p>
The first patch only applies with fuzz 2, because of <a class="closed ticket" href="https://trac.sagemath.org/ticket/11431" title="defect: Conversion from Singular to Sage (closed: fixed)">#11431</a> (-> new dependency). The conflict is in the author list, easy to fix.
</p>
TicketSimonKingSat, 03 Mar 2012 08:16:33 GMTstatus, description changed; work_issues deleted
https://trac.sagemath.org/ticket/10296#comment:60
https://trac.sagemath.org/ticket/10296#comment:60
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>rebase</em> deleted
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10296?action=diff&version=60">diff</a>)
</li>
</ul>
<p>
I produced a combined patch and verified that it cleanly applies on sage-5.0.beta6.
</p>
<p>
I started doctests without the patch, and later I will repeat with the patch. But for now it needs review...
</p>
<p>
Apply trac10296_singular_overhead_combined.patch
</p>
TicketSimonKingSat, 03 Mar 2012 11:45:20 GMT
https://trac.sagemath.org/ticket/10296#comment:61
https://trac.sagemath.org/ticket/10296#comment:61
<p>
FWIW: The doc tests of sage-5.0.beta6 with the combined patch applied pass.
</p>
TicketmalbSat, 03 Mar 2012 13:41:39 GMT
https://trac.sagemath.org/ticket/10296#comment:62
https://trac.sagemath.org/ticket/10296#comment:62
<p>
I only have some very minor comments:
</p>
<ul><li>Could you escape EOFError with "<code></code>" backquotes so it's printed as code
</li><li>You wrote: "Since nesting of _eval_line can not occur during garbage collection" can you explain that, I am not sure I understand.
</li><li>why did you indent REMARK etc. further in pid()?
</li></ul>
TicketSimonKingSat, 03 Mar 2012 14:32:58 GMT
https://trac.sagemath.org/ticket/10296#comment:63
https://trac.sagemath.org/ticket/10296#comment:63
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:62" title="Comment 62">malb</a>:
</p>
<blockquote class="citation">
<p>
I only have some very minor comments:
</p>
<ul><li>Could you escape EOFError with "<code></code>" backquotes so it's printed as code
</li></ul></blockquote>
<p>
Done
</p>
<blockquote class="citation">
<ul><li>You wrote: "Since nesting of _eval_line can not occur during garbage collection" can you explain that, I am not sure I understand.
</li></ul></blockquote>
<p>
I elaborated the comment.
</p>
<p>
Without my patch, garbage collection would result in a call to _eval_line. So, if garbage collection occurs while being in _eval_line, we would have two nested calls to _eval_line. The inner _eval_line would receive the Singular prompt and return. The outer _eval_line would also wait for a Singular prompt -- but Singular would not provide a second prompt. Result: A deadlock.
</p>
<p>
To my understanding, this is why synchronisation of the Singular interface was necessary. But with my patch, garbage collection does not involve an additional call to _eval_line, and thus the deadlock is prevented without synchronisation.
</p>
<blockquote class="citation">
<ul><li>why did you indent REMARK etc. further in pid()?
</li></ul></blockquote>
<p>
To the contrary, the first line of the doc string of pid() needed to be indented more! Fixed with the new patch version.
</p>
<p>
Apply trac10296_singular_overhead_combined.patch
</p>
TicketSimonKingSat, 03 Mar 2012 14:38:45 GMTattachment set
https://trac.sagemath.org/ticket/10296
https://trac.sagemath.org/ticket/10296
<ul>
<li><strong>attachment</strong>
set to <em>trac10296_singular_overhead_combined.patch</em>
</li>
</ul>
<p>
Modify garbage collection in Singular interface, which reduces the overhead. Synchronization of Gap interface. Cope with non-linux pexpect errors
</p>
TicketSimonKingSat, 03 Mar 2012 14:40:27 GMT
https://trac.sagemath.org/ticket/10296#comment:64
https://trac.sagemath.org/ticket/10296#comment:64
<p>
Oops, I only had a single backtick around <code>EOFError</code> -- hence, it would be typeset as <code>LaTeX</code>. So, I have updated my patch, with double backticks around <code>EOFError</code>.
</p>
TicketmalbSat, 03 Mar 2012 15:22:16 GMTstatus changed
https://trac.sagemath.org/ticket/10296#comment:65
https://trac.sagemath.org/ticket/10296#comment:65
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I am satisfied.
</p>
<p>
But out of curiosity, do you have a simple before/after benchmark you could add to this *ticket*?
</p>
TicketjdemeyerTue, 13 Mar 2012 08:21:51 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10296#comment:66
https://trac.sagemath.org/ticket/10296#comment:66
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.0.beta8</em>
</li>
</ul>
TicketjdemeyerSun, 18 Mar 2012 11:23:02 GMT
https://trac.sagemath.org/ticket/10296#comment:67
https://trac.sagemath.org/ticket/10296#comment:67
<p>
There is a doctest error on <a class="missing wiki">OpenSolaris?</a>, please have a look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/12687" title="defect: Fix Singular doctest error on OpenSolaris (closed: fixed)">#12687</a>.
</p>
TicketjdemeyerWed, 21 Mar 2012 13:54:01 GMT
https://trac.sagemath.org/ticket/10296#comment:68
https://trac.sagemath.org/ticket/10296#comment:68
<p>
I have also seen the following error:
</p>
<pre class="wiki">sage -t --long -force_lib devel/sage/sage/interfaces/expect.py
**********************************************************************
File "/home/buildbot/build/sage/cleo-1/cleo_full/build/sage-5.0.beta9/devel/sage-main/sage/interfaces/expect.py", line 829:
sage: singular.interrupt(timeout=1)
Expected:
False
Got:
True
**********************************************************************
</pre><p>
What does that mean? It is really a failure or should we just increase the timeout?
</p>
TicketSimonKingWed, 21 Mar 2012 14:56:51 GMT
https://trac.sagemath.org/ticket/10296#comment:69
https://trac.sagemath.org/ticket/10296#comment:69
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:68" title="Comment 68">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
What does that mean? It is really a failure or should we just increase the timeout?
</p>
</blockquote>
<p>
That error has always been mysterious to me.
</p>
<p>
Frankly, I need a break, in the sense of I need to focus on my main project (cohomology and ext algebras) and on my family. Martin, would you be able to take over?
</p>
TicketleifWed, 21 Mar 2012 15:13:42 GMT
https://trac.sagemath.org/ticket/10296#comment:70
https://trac.sagemath.org/ticket/10296#comment:70
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:69" title="Comment 69">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10296#comment:68" title="Comment 68">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
What does that mean? It is really a failure or should we just increase the timeout?
</p>
</blockquote>
<p>
That error has always been mysterious to me.
</p>
<p>
Frankly, I need a break, in the sense of I need to focus on my main project (cohomology and ext algebras) and on my family.
</p>
</blockquote>
<p>
Yes, have a "break". :-)
</p>
<p>
<br />
</p>
<p>
Since cleo is not the fastest machine, and I assume the above occurred when doctesting in parallel, I would either ignore it (if it doesn't happen too often), or try with increasing the timeout (to a few seconds) and if that helps, change the doctest permanently.
</p>
<p>
For the test I don't think it matters whether we wait 1 or 5 seconds, probably even more; it's only important that Singular finally <em>does</em> get interrupted (within perhaps at most half a minute, say).
</p>
Ticket