Pipes need cleaned

[jvoight]

Our pipe commands through Pari/gp using Pipe("gp -q",cmd) work fine interactively, but in batch mode it fails in various places because the line "time X ms." gets added to the output for some reason. The code is blindly evaluating the return value from Pipe, so it breaks. We should be more careful with processing the output from gp, at least removing all lines containing the string "time" from the result.

Reported by Drew Sutherland.

[JRSijsling]

I am not familiar enough with Pari to solve this, and will ask Nicolas. In the meantime note that all functions referring to Pari are in polredabs.m and roots.m in endomorphisms/magma, so no need to look anywhere else for problems to fix.

[edgarcosta]

@AndrewVSutherland, can you provide an example with traceback?

[AndrewVSutherland]

Just add timer = 1 to your .gprc and try calling HeuristicEndomorphismLattice with any input that takes more than a millisecond. e.g.

> F := RationalsExtra(200);          
> R<x> := PolynomialRing(F);
> HeuristicEndomorphismLattice(HyperellipticCurve(R![18,2,0,0,0,2,0,0,8]));                          
In file "/home/drew/Dropbox/github/endomorphisms/endomorphisms/magma/roots.m", line 11, column 25:
>> rts := [ K ! rt : rt in eval(s) ];
                           ^

In eval expression, line 2, column 1:
>> time = 4 ms.;
   ^
Located in file "/home/drew/Dropbox/github/endomorphisms/endomorphisms/magma/roots.m", at line 11, column 25:
>> rts := [ K ! rt : rt in eval(s) ];
                           ^
User error: bad syntax

I still do not understand why the timer was getting turned on when I ran jobs in batch mode (it is supposed to be off by default), but in any case your code should not break just because someone has timer = 1 set in the .gprc.

An easy fix is to ignore lines containing the string "time" in the return value from pipe -- I worked around this problem using the wrapper

function MyPipe(cmd,args) s:=Pipe(cmd,args); return [t:t in Split(s)|not "time" in t][1]; end function;

I then did a global search/replace to change Pipe to MyPipe. But this is a stupid fix, it would be better if your code parsed the return value from Pipe to explicitly grab what it is looking for rather than doing a blind eval on the whole string (which could easily break in a future release of gp). For example in the cmf code the magma implementation of Polredabs looks like:

function get_gp_coeffs(s)
    return [StringToInteger(x) : x in Split(s[Index(s,"[")+1..Index(s,"]")-1],",")];
end function;

intrinsic Polredabs(f::SeqEnum:DiscFactors:=[]) -> SeqEnum
{ Computes a smallest canonical defining polynomial of the etale algebra Q[x]/(f(x)) using pari. }
    cmd := #DiscFactors eq 0 select Sprintf("{print(Vecrev(Vec(polredabs(Pol(Vecrev(%o))))))}", f) else  Sprintf("{print(Vecrev(Vec(polredabs([Pol(Vecrev(%o)),%o]))))}", f,DiscFactors);
    s := Pipe("gp -q", cmd);
    return get_gp_coeffs(s);
end intrinsic;

This implementation certainly isn't bullet proof, but it is a lot less brittle than calling eval and if it fails the user will know exactly where and what the problem is.

[nmascot]

How about Pipe("gp -q -D timer=0",cmd) ? According to the man page, this overrides the .gprc settings. This is not bullet proof either though, as it will not work it .gprc contains an instruction to read a .gp file (typically, .gprc.gp) containing default(timer,1), which is not that uncommon (see for instance http://pari.math.u-bordeaux.fr/Events/PARI2019/talks/sources.pdf). I'm not sure if that is the intended behavior, I'll ask Bill. Edit according to Bill, setting the timer in .gprc.gp (instead of .gprc) is uncommon. My bad.

[nmascot]

Here is a better suggestion from Bill: Pipe("gp -qf",cmd) -f means .gprc is ignored altogether. Since timer=0 is the default option, this should solve the problem.

[AndrewVSutherland]

I'm not sure that is a great idea, you will often want parisize to be set and it will be very confusing to the user if they keep seeing messages about stack size that they don't expect to see given how they have parisize set in their gprc.

[nmascot]

Ah, good point. How about -D timer=0 then? (according to Bill, good practice dictates that the timer should be set in .gprc, not .gprc.gp; and then -D will override .gprc)

[AndrewVSutherland]

Just to bump this, it would be great if you could replace "gp -q" with "gp -q -D timer=0" in polredabs.m and roots.m (three times) so that I don't have to remember to do this every time I clone or pull your repo.

[JRSijsling]

Just pushed a commit. Let me know if it works.

[AndrewVSutherland]

Modulo wrapping output in print (see PR #27), gp piping works fine, you can close this issue.