sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 466 forks source link

Get PARI/GP to stop starting automatically #9959

Closed kcrisman closed 13 years ago

kcrisman commented 14 years ago

I still am getting the known behavior below when exiting Sage.

Exiting Sage (CPU time 0m0.74s, Wall time 11m11.95s). 
Exiting spawned GP/PARI interpreter process. 

It's not exactly a bug, but it's also annoying and potentially confusing. We should stop it.

Apply 9959_combined.patch below and install the new spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg (note that this spkg is not based on the earlier p8)

CC: @qed777 @jdemeyer @nexttime

Component: packages: standard

Author: Jeroen Demeyer

Reviewer: John Cremona

Merged: sage-4.6.rc0

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

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:1

Oh, I thought you just meant "GP/PARI" should read "PARI/GP"...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:2

So how do we manage this?

Rewrite the class sage.interfaces.gp.Gp to lazily start the interpreter, or initialize sage.interfaces.gp.gp to None and explicitly do gp = Gp() everywhere it is used unless gp != None?

jdemeyer commented 14 years ago
comment:3

I believe this in Gp.__init__ is the culprit:

# gp starts up with this set to 1, which makes pexpect hang: 
self.set_default('breakloop',0)

If this is the case, an easy solution is to add a patch to the pari spkg to set breakloop to 0 by default.

jdemeyer commented 14 years ago
comment:4

Replying to @nexttime:

Oh, I thought you just meant "GP/PARI" should read "PARI/GP"...

This should also be changed.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:5

Replying to @jdemeyer:

I believe this in Gp.__init__ is the culprit:

# gp starts up with this set to 1, which makes pexpect hang: 
self.set_default('breakloop',0)

Good catch. I read this a dozen times without noticing that this actually calls gp... 8/

If this is the case, an easy solution is to add a patch to the pari spkg to set breakloop to 0 by default.

Try it...

jdemeyer commented 14 years ago

Attachment: 9959_rename_pari_gp.patch.gz

Renames "GP/PARI" to "PARI/GP"

jdemeyer commented 14 years ago
comment:6

New spkg for testing: http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p8.spkg

jdemeyer commented 14 years ago

Author: Jeroen Demeyer

jdemeyer commented 14 years ago

Description changed:

--- 
+++ 
@@ -5,6 +5,6 @@
 Exiting spawned GP/PARI interpreter process. 

It's not exactly a bug, but it's also annoying and potentially -confusing to a non-Pari user. We should stop it. +confusing. We should stop it.

-This is with 4.6.alpha1.
+Apply all 3 patches below and the new spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p8.spkg

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:8

Wow, a lot of places where "GP/PARI" had to be changed... (well done)

We have two similar instances in the scripts repo:

local/bin/sage-sage:    echo "  -gp <..>      -- run Sage's GP (Pari) with given arguments"
local/bin/sage-sage.py:                     help="run Sage's GP (Pari), passing it all remaining arguments")

But I think we don't need to change these on this ticket.

Patches look fine, will test them later (with the new spkg of course).

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:9

The spkg's changelog entry (and patches/README.txt) could describe the reason for patching default.c a bit more. (The ticket number is only in the commit message.)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:10

s|and|and/or|

JohnCremona commented 14 years ago
comment:11

OK, so I was the one who put in those two breakloop calls, which I only did since otherwise things did not work properly -- but in the testing I just did I found it only necessary to delete the brealoop call in __init__, while keeping the one in _start. Are you both sure that it is safe to delete the one in _start as well? Does that not cause gp to enter the breakloop on encountering an error, which is not what you want?

If you are right (and I assumed that you did test this!) then I am wondering what else has changed, since when we started the pari upgrade it was definitely necessary to deal with this.

And by the way, the only reason I created a _start function for Gp different from the default one in PExpect was to add this line in _start; so if that line is not needed, you can probably delete the while _start function for class Gp.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:12

I haven't come to testing yet... (including a closer look at the relevant PARI sources).

It should at least not hurt to keep the second set_default('breakloop',0) in Gp._start().

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:13

Replying to @nexttime:

It should at least not hurt to keep the second set_default('breakloop',0) in Gp._start().

As far as I can see, we don't need it there, since gp will of course (re)start with the hard-coded defaults whenever _start() is called.

(This is different to setting it "manually" from the Expect interface each time gp is [re]started.)

So we can IMHO drop Gp._start() completely, as John noted.

jdemeyer commented 14 years ago
comment:14

Replying to @JohnCremona:

Are you both sure that it is safe to delete the one in _start as well? Does that not cause gp to enter the breakloop on encountering an error, which is not what you want?

The new spkg patches the gp source code to disable the breakloop by default, so we can remove all references to breakloop from Sage. Note also that the patch attachment: 9959_gp_error_doctest.patch actually acts a doctest to check this.

jdemeyer commented 14 years ago

Don't set the 'breakloop' default in Sage

jdemeyer commented 14 years ago

Attachment: 9959_remove_breakloop.patch.gz

Attachment: 9959_gp_error_doctest.patch.gz

Add doctest for error recovery

jdemeyer commented 14 years ago
comment:15

Replying to @JohnCremona:

And by the way, the only reason I created a _start function for Gp different from the default one in PExpect was to add this line in _start; so if that line is not needed, you can probably delete the while _start function for class Gp.

Done

JohnCremona commented 14 years ago
comment:16

It's a pity that we had to patch the gp source code -- clearly that was an option right from the start, and what I did was to avoid that. Best (in my opinion) would be if gp had a command-line option to turn off the breakloop default, since Sage could easily use that. It might be worth suggesting that possibility upstream.

jdemeyer commented 14 years ago

Attachment: pari.p8.patch.gz

Patch for the PARI spkg .p7 to .p8 (for review)

jdemeyer commented 14 years ago
comment:17

Replying to @JohnCremona:

It's a pity that we had to patch the gp source code -- clearly that was an option right from the start, and what I did was to avoid that. Best (in my opinion) would be if gp had a command-line option to turn off the breakloop default, since Sage could easily use that. It might be worth suggesting that possibility upstream.

If one would add an option to gp, add an option for non-interactive (script) use which would also disable breakloop by default. I remember I proposed this (a long time ago) to the PARI/GP developers without much success.

JohnCremona commented 14 years ago
comment:18

Replying to @jdemeyer:

Replying to @JohnCremona:

It's a pity that we had to patch the gp source code -- clearly that was an option right from the start, and what I did was to avoid that. Best (in my opinion) would be if gp had a command-line option to turn off the breakloop default, since Sage could easily use that. It might be worth suggesting that possibility upstream.

If one would add an option to gp, add an option for non-interactive (script) use which would also disable breakloop by default. I remember I proposed this (a long time ago) to the PARI/GP developers without much success.

I suppose that from their point of view it makes no sense to use gp non-interactively, since gp is the interactive interface to the pari library!

jdemeyer commented 14 years ago
comment:19

Alternative solution:

When gp starts, it reads a configuration file (by default, $HOME/.gprc). If the environment variable $GPRC is set, it uses that as a filename for the .gprc file. We could create a file $HOME/.sage/gp/gprc similarly to $HOME/.sage/ipyhton/ipythonrc and have Sage set $GPRC to that location.

Then, the file $HOME/.sage/gp/gprc should contain a line:

breakloop =  0
jdemeyer commented 14 years ago
comment:20

Replying to @JohnCremona:

I suppose that from their point of view it makes no sense to use gp non-interactively, since gp is the interactive interface to the pari library!

Good point :-)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:21

Replying to @jdemeyer:

Alternative solution:

When gp starts, it reads a configuration file (by default, $HOME/.gprc). If the environment variable $GPRC is set, it uses that as a filename for the .gprc file. We could create a file $HOME/.sage/gp/gprc similarly to $HOME/.sage/ipyhton/ipythonrc and have Sage set $GPRC to that location.

Then, the file $HOME/.sage/gp/gprc should contain a line:

breakloop =  0

I was just going to suggest that, too.

Note that we should have two of them, one for the interactive gp provided by Sage, and one for the pexpect interface.

jdemeyer commented 14 years ago
comment:22

Replying to @nexttime:

Note that we should have two of them, one for the interactive gp provided by Sage, and one for the pexpect interface.

I think we need only one, for the pexpect interface. I would prefer sage -gp to use my $HOME/.gprc.

Setting this to needs_work to implement the alternative solution.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:23

Replying to @jdemeyer:

If one would add an option to gp, add an option for non-interactive (script) use which would also disable breakloop by default. I remember I proposed this (a long time ago) to the PARI/GP developers without much success.

Perhaps they meanwhile chnaged their minds.. ;-)

There's some stuff in it for using gp from Emacs or TeXmacs, but only as a compile time option IIRC. Also some kind of "non-interactive" gp use.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:24

Replying to @jdemeyer:

Replying to @nexttime:

Note that we should have two of them, one for the interactive gp provided by Sage, and one for the pexpect interface.

I think we need only one, for the pexpect interface. I would prefer sage -gp to use my $HOME/.gprc.

Right, but we should take care not to use the other one (i.e. by setting GPRC and therefore overriding $HOME/.gprc) if we start the interactive gp.

jdemeyer commented 14 years ago
comment:25

Anybody happens to know where is the code to deal with $DOT_SAGE, to populate it with files?

jdemeyer commented 14 years ago

Use $SAGE_ROOT/local/etc/gprc.expect as .gprc file

jdemeyer commented 14 years ago
comment:26

Attachment: 9959_gprc.patch.gz

New spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg. This is based on p7 (not p8) and adds the gprc.expect file. So installing the new spkg and applying all 4 patches should do it.

jdemeyer commented 14 years ago

Description changed:

--- 
+++ 
@@ -7,4 +7,4 @@
 It's not exactly a bug, but it's also annoying and potentially 
 confusing.  We should stop it.

-Apply all **3 patches** below and the **new spkg**: [http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p8.spkg](http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p8.spkg)
+Apply all **4 patches** below and install the **new spkg**: [http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg](http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg) (note that this spkg is *not* based on the earlier p8)
jdemeyer commented 14 years ago
comment:28

Doesn't work yet, stupid mistake.

jdemeyer commented 14 years ago
comment:29

sage.math.washington.edu seems much slower than usual today...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:30

Does gp expand $SAGE_ROOT in the gprc.expect file, or does Sage do that before it starts gp?

jdemeyer commented 14 years ago
comment:31

Replying to @nexttime:

Does gp expand $SAGE_ROOT in the gprc.expect file, or does Sage do that before it starts gp?

gp does that.

jdemeyer commented 14 years ago

.gprc file for the Gp() interface

jdemeyer commented 14 years ago

Attachment: gprc.expect.gz

Attachment: pari.p9.patch.gz

Patch for the PARI spkg .p7 to .p9 (for review)

jdemeyer commented 14 years ago
comment:32

Fixed spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg

jdemeyer commented 14 years ago

Attachment: 9959_combined.patch.gz

All 4 sagelib patches combined

jdemeyer commented 14 years ago

Description changed:

--- 
+++ 
@@ -7,4 +7,4 @@
 It's not exactly a bug, but it's also annoying and potentially 
 confusing.  We should stop it.

-Apply all **4 patches** below and install the **new spkg**: [http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg](http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg) (note that this spkg is *not* based on the earlier p8)
+Apply **9959_combined.patch** below and install the **new spkg**: [http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg](http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg) (note that this spkg is *not* based on the earlier p8)
JohnCremona commented 14 years ago
comment:35

The sagelib patch looks good to me.

For review, please could you give instructions for the simple-minded as to how to apply the patch which converts .p7 to .p9? And then how to rebuild? Thanks.

Of course, someone else who knows how to do this reliably is welcome to get in first with a review.

jdemeyer commented 14 years ago
comment:36

Replying to @JohnCremona:

The sagelib patch looks good to me.

For review, please could you give instructions for the simple-minded as to how to apply the patch which converts .p7 to .p9?

Well, the easiest is to install the new spkg:

sage -i http://sage.math.washington.edu/home/jdemeyer/spkg/pari-2.4.3.svn-12577.p9.spkg

And then how to rebuild?

I personally do not know the most reliable way to rebuild a complete Sage installation. One thing which works for sure is the following:

jdemeyer commented 14 years ago

Reviewer: John Cremona

JohnCremona commented 14 years ago
comment:37

THanks -- I am doing that, will do a complete new build (with the sagelib patch too of course) and report back.

JohnCremona commented 14 years ago
comment:38

I made a freshly unpacked 4.6.alpha3, and replaced its pari spkg with the p9 version; then built all Sage; then applied the sagelib patch on this ticket; then tested the whole library.

All passed, so here's a positive review.

e14f4152-4982-4ace-8c95-73a0599b109b commented 13 years ago

Merged: sage-4.6.rc0