Opened 4 years ago
Closed 4 years ago
#23913 closed defect (fixed)
Doctest failures with GMP
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  packages: optional  Keywords:  
Cc:  fbissey, mderickx  Merged in:  
Authors:  Maarten Derickx  Reviewers:  Jeroen Demeyer 
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  
Branch:  bf70564 (Commits, GitHub, GitLab)  Commit:  bf7056499f7e4c9a7fca342523238a27144cc8c9 
Dependencies:  Stopgaps: 
Description
With GMP is used instead of MPIR, there are two doctest failures because of error checking that GMP does but MPIR does not:
********************************************************************** File "src/sage/rings/integer.pyx", line 6099, in sage.rings.integer.Integer._shift_helper Failed example: 1 << (2^60) Expected: Traceback (most recent call last): ... MemoryError: failed to allocate ... bytes Got: gmp: overflow in mpz type Traceback (most recent call last): File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 515, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 885, in compile_and_execute exec(compiled, globs) File "<doctest sage.rings.integer.Integer._shift_helper[8]>", line 1, in <module> Integer(1) << (Integer(2)**Integer(60)) File "sage/rings/integer.pyx", line 6177, in sage.rings.integer.Integer.__lshift__ (build/cythonized/sage/rings/integer.c:39270) return (<Integer>x)._shift_helper(y, 1) File "sage/rings/integer.pyx", line 6138, in sage.rings.integer.Integer._shift_helper (build/cythonized/sage/rings/integer.c:39014) sig_on() RuntimeError: Aborted **********************************************************************
and
sage t warnlong 61.3 src/sage/ext/memory.pyx ********************************************************************** File "src/sage/ext/memory.pyx", line 9, in sage.ext.memory Failed example: 2^(2^632) Expected: Traceback (most recent call last): ... MemoryError: failed to allocate 1152921504606847008 bytes Got: gmp: overflow in mpz type Traceback (most recent call last): File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 515, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 885, in compile_and_execute exec(compiled, globs) File "<doctest sage.ext.memory[0]>", line 1, in <module> Integer(2)**(Integer(2)**Integer(63)Integer(2)) File "sage/rings/integer.pyx", line 2067, in sage.rings.integer.Integer.__pow__ (build/cythonized/sage/rings/integer.c:14173) sig_on() RuntimeError: Aborted **********************************************************************
Upstream GMP provides no way to hook this error: https://gmplib.org/listarchives/gmpdiscuss/2017September/006144.html
Attachments (1)
Change History (24)
comment:1 Changed 4 years ago by
 Cc fbissey added
comment:2 Changed 4 years ago by
 Cc mderickx added
comment:3 Changed 4 years ago by
comment:4 Changed 4 years ago by
That's an interesting idea considering that by default tests are run with optional=mpir,python2,sage
so the default behavior isn't changed. But I am not sure what happens when gmp
is selected. But I think that is a good approach.
comment:5 Changed 4 years ago by
The default argument for optional is actually determined programmatically. sage
is always there and the rest is just a list of optional packages. It just happens that python2 and mpir are optional packages that are installed by default. If you would build sage with python3 and gmp instead it will be optional=gmp,python3,sage
instead.
comment:6 Changed 4 years ago by
I was not sure of the mechanics but it makes sense for it be that way. Your solution should work. I'll be waiting to see your suggested branch.
comment:7 Changed 4 years ago by
 Branch set to public/23913
 Commit set to beef1a813eecf7d24de8f05ec839f824dc5275ee
Ok, I created a branch with how I think the fix should look like. But I did not put it to needs review yet because I do not understand how to check that it works, the main problem is that I do not now how to properly build sage with gmp.
I tried just:
sage i gmp
in a sage that was already build since I was hoping to save time, but this does not work (it is running the optional gmp test and not the mpir one, but the test result is that of mpir). Should I do:
sage i gmp make build
Or do I need to do:
make distclean make gmp make build
New commits:
beef1a8  Trac #23913: Fixed doctest failures with GMP

comment:8 Changed 4 years ago by
I am not sure from an existing sage I am afraid, rebuilding is necessary and there could be stray libraries in the way. It would better and much cleaner  but unfortunately more time consuming  to do
./configure withmp=gmp make
comment:9 Changed 4 years ago by
 Status changed from new to needs_info
comment:10 Changed 4 years ago by
 Status changed from needs_info to needs_work
I got an error that configure was not found in a clean install, so I had to do
make configure ./configure withmp=gmp make
I am now building and waiting for how the tests turn out. If you already have an install with gmp lying around and you test this branch there then you will probably know the results before I do.
Changed 4 years ago by
comment:11 Changed 4 years ago by
The commands:
make configure ./configure withmp=gmp make
resulted in the failure building of mpfr on OS X 10.12.4 with Xcode 8.3.3 before gcc was build. I've attached the log, so I still don't know how to test it on this machine. I can test it on Ubuntu tomorrow.
comment:12 Changed 4 years ago by
That's still a problem thanks for pointing it out. We didn't notice it on the gmp
upgrade ticket but we should have.
comment:13 Changed 4 years ago by
Ok, I have now noticed something. You are not using the latest gmp
from the gmp
upgrade ticket. It would be best if that was the case. Since it is not in a beta yet you should depend on #19706 and merge its branch with the one in this ticket. It looks like there is an issue with the produced libgmp with your version of xcode and it is quite possible it is fixed by the gmp version of #19706.
comment:14 Changed 4 years ago by
It took me a while to figure out how to pull #19706, but it now works and gets me past mpfr. The build is now at the point of building gcc. I'll report back tomorrow.
comment:15 Changed 4 years ago by
Good, that means that gmp
before the upgrade was broken on OS X anyway, so it was high time to upgrade it.
comment:16 Changed 4 years ago by
Your patch makes sense, but you will need to specialcase 32bit. I'm pretty sure that the 32bit result is an OverflowError
both with mpir
and with gmp
.
comment:17 Changed 4 years ago by
Thanks for pointing this out, I am now building sage with the gmp from #19706 on a 32bit machine as well to see what the exact error message looks like.
comment:18 Changed 4 years ago by
The OverflowError
is raised by Cython, so it would be the same for mpir
and gmp
.
comment:19 Changed 4 years ago by
 Commit changed from beef1a813eecf7d24de8f05ec839f824dc5275ee to dcb984a4f99eefbfbd5064bf82fda815482c601d
Branch pushed to git repo; I updated commit sha1. New commits:
dcb984a  Trac 23913: Fixed doctest failures with GMP in 32bit

comment:20 Changed 4 years ago by
 Status changed from needs_work to needs_review
Ok, I tested the current branch on:
 32bit gmp + #19706
 32bit mpir
 64bit mpir
I still need to test 64bit gmp + #19706 since the build on my laptop got interrupted a few times, I think it should pass but I am not 100% sure. I am hesitant to put it into needs review before I know this case passes, however if either of you can verify that this case also passes then feel free to review this ticket.
I don't think this ticket needs a dependency on #19706 since it merges cleanly with it.
comment:21 Changed 4 years ago by
 Branch changed from public/23913 to public/23913_clean
 Commit changed from dcb984a4f99eefbfbd5064bf82fda815482c601d to bf7056499f7e4c9a7fca342523238a27144cc8c9
Ok the case: 64bit gmp + #19706 failed. I didn't realise that the doctesting framework only sees errors as errors if the error is thrown on the first line.
So the expected/got in
Expected: Traceback (most recent call last): ... MemoryError: failed to allocate ... bytes Got: gmp: overflow in mpz type Traceback (most recent call last): File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 515, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/jdemeyer/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 885, in compile_and_execute exec(compiled, globs) File "<doctest sage.rings.integer.Integer._shift_helper[8]>", line 1, in <module> Integer(1) << (Integer(2)**Integer(60)) File "sage/rings/integer.pyx", line 6177, in sage.rings.integer.Integer.__lshift__ (build/cythonized/sage/rings/integer.c:39270) return (<Integer>x)._shift_helper(y, 1) File "sage/rings/integer.pyx", line 6138, in sage.rings.integer.Integer._shift_helper (build/cythonized/sage/rings/integer.c:39014) sig_on() RuntimeError: Aborted
Is a bit misleading since for the passing doctest for this is without the "gmp: overflow in mpz type" line.
Traceback (most recent call last): ... RuntimeError: Aborted
anyway. It now passes in all 4 cases for me, so ready for review.
New commits:
bf70564  Trac #23913: Fixed doctest failures with GMP

comment:22 Changed 4 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:23 Changed 4 years ago by
 Branch changed from public/23913_clean to bf7056499f7e4c9a7fca342523238a27144cc8c9
 Resolution set to fixed
 Status changed from positive_review to closed
Since both mpir and gmp are optional packages couldn't we also implement a fix by having
in the mean time? If someone here at this ticket thinks that it is a good idea, then I will create a ticket with the relatively easy patch for it.