magma mode in 4.7 notebook broken

[nbruin]

See also ​this sage-devel thread

It seems that magma mode in the 4.7 notebook is broken. I observed the following session after verifying John Cremona's report about syncing problems. I suspect sync is lost due to the multiple commands. Note that none of the print commands seem to get executed (no output is observed) and that for "print 7;" the line "print 6;" gets echoed (this has probably very little to do with the "print 7;" command itself)

a:=1;
b:=2;
c:=3;
///
b:=2;
c:=3;

print 4;
///

d:=4;
print 5;
///

print 6;
///

print 7;
///
print 6;

---

Apply only trac_11401v2.2.patch​ to the Sage library.

[jdemeyer]

Could this be caused by #9705?

[nbruin]

Could this be caused by #9705?

I guess it could be caused by its fix ... The cells there execute without problem for me in 4.7 and those cells do contain multiple commands that do not produce output. It seems that the input (in one cell)

a:=1;
b:=2;

reliably knocks the interface out of sync. So probably if someone who know how these interfaces work, looks at the strings that are passed back and forth, it will become apparent quite quickly.

I don't think the problem is confined to the notebook either. I observed:

sage: magma.execute("""
....: a:=1;
....: b:=2;
....: """)
'b:=2;'
sage: magma.execute("print 3;")
''

[nbruin]

straightforward fix

[nbruin]

Perhaps a little too naive of a fix and perhaps focussing a little too much on the symptom rather than the cause, but if newlines are causing problems, why not just replace them with spaces? In magma syntax, both newline and space are whitespace characters and those are all treated the same. Attached fix does pass

sage -t --optional --long magma.py

which I suppose contains all magma interfacing doctests?

[nbruin]

I think I now have a proper fix for the problem. The problem is indeed caused by the fix of #9705. The matter is the following (and this will be true for a lot of interfaces):

• Magma essentially responds to each newline with a new prompt. So, when you are feeding input to magma via its stdin, the most straightforward way is to do this line by line and eat the prompts as you go along. This happened before the fix of #9705.
• The decision to transfer via file or via stdin was made on a line-by-line basis. So, newlines inside expressions could lead to input being split over several files and/or a bit of stdin. This caused #9705. The fix introduced there solves the problem by introducing split_lines. With split_lines=False, one can end up with multi-line input in expect._eval_line. When _eval_line decides to communicate those lines via stdin then there is a mismatch between the number of prompts generated and the number expected.

The solution in trac_11401v2.patch is to move the decision to use files to "eval". This makes it possible to either transmit all code via a temp file or to send the input line-by-line to stdin (and those lines will never trigger file use). I think this is also more efficient because it favours transfer in bigger blocks (before the #9705 fix, input consisting of many short lines would still be communicated via stdin. Now it will be communicated via one tmp file)

This means that the split_lines option can now take 3 values. The option "use one file OR split lines" is the right behaviour for magma and probably for most other interfaces too.

I have also added a doctest testing that #11401 is fixed. I also reintroduced the newlines in the test for #9705. Those are actually essential for testing (the version without newlines always worked as expected).

[mstreng]

I don't have a machine without Magma to test this on, but I'm pretty sure that the examples "sage: magma.eval("a:=3;\nb:=5;")" and "magma.eval("[a,b];")" need "# optional - magma".

ps. for the patchbot which didn't seem to have gotten the message: apply trac_11401v2.patch

[nbruin]

(commit message + "optional -- magma" for examples)

[nbruin]

Good catch! fixed.

(amnesic patchbot: apply trac_11401v2.patch)

[mstreng]

Why are you changing the example for "Verify that trac 9705 is fixed"? I don't see why one is better than the other.

To check for myself that your new version of that test really verifies that trac 9705 is fixed, I would have to install a pre-#9705 version of Sage and see that that test fails. That's a bit more work than I would like to do.

Patchbot, would you apply trac_11401v2.patch please?

[mstreng]

All tests pass. Patch looks good. I haven't tried the notebook, but the patch does what it should do to multi-line magma.eval.

That leaves just the one question about the changed #9705 doctest.

[nbruin]

Replying to mstreng:

Why are you changing the example for "Verify that trac 9705 is fixed"? I don't see why one is better than the other.

Because the example without newlines runs OK both with and without the fix of #9705, so the test is not verifying any change in behaviour. The problem described in #9705 arose when one magma statement was spread over multiple lines and one line was passed over stdin and another line via a "load" command. Without newlines, such splits wouldn't occur anyway.

If people can't find a fix for the "\n" in the tests, I guess we could write it in the following way:

sage: import commands
sage: newline=commands.getoutput("echo; echo")
sage: command=(
        "_<x>:=PolynomialRing(Rationals());"+newline+
        "repeat"+newline+
        "  g:=3*b*x^4+18*c*x^3-6*b^2*x^2-6*b*c*x-b^3-9*c^2 where b:=Random([-10..10]) where c:=Random([-10..10]);"+newline+
        "until Roots(g) ne [];")
sage: magma.eval(command)

[nbruin]

compliant commit-message and fix for newlines

[nbruin]

OK, latest version should provide proper formatting of documentation _and_ have newlines in doctests. The doc formatting is surprisingly fragile in the face of multi-line strings, so manually adding the newline characters seems to be the best idea. Plus, second lines of multiline strings get preparsed by the doctester! That wreaks havoc with magma code (Integer(3) etc.)

Dear patchbot, would you be so kind to just apply trac_11401v2.2.patch please?

[mstreng]

That new version looks good, and does the trick.

I'm glad you didn't really do newline=commands.getoutput("echo; echo")

[leif]

Please use proper URLs, or in this case, trac wiki mark-up ("[attachment:here_comes_the_filename]") to reference attached patches in the description.