Opened 2 years ago

Closed 2 years ago

#23913 closed defect (fixed)

Doctest failures with GMP

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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) 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/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/jdemeyer/sage/local/lib/python2.7/site-packages/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 --warn-long 61.3 src/sage/ext/memory.pyx
**********************************************************************
File "src/sage/ext/memory.pyx", line 9, in sage.ext.memory
Failed example:
    2^(2^63-2)
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/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/jdemeyer/sage/local/lib/python2.7/site-packages/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/list-archives/gmp-discuss/2017-September/006144.html

Attachments (1)

mpfr-3.1.5.p0.log (622.1 KB) - added by mderickx 2 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 2 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 2 years ago by mderickx

  • Cc mderickx added

comment:3 Changed 2 years ago by mderickx

Since both mpir and gmp are optional packages couldn't we also implement a fix by having

2^(2^63-2) #optional mpir
some error
2^(2^63-2) #optional gmp
another error

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.

comment:4 Changed 2 years ago by fbissey

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 2 years ago by mderickx

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 2 years ago by fbissey

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 2 years ago by mderickx

  • Authors set to Maarten Derickx
  • 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:

beef1a8Trac #23913: Fixed doctest failures with GMP
Last edited 2 years ago by mderickx (previous) (diff)

comment:8 Changed 2 years ago by fbissey

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 --with-mp=gmp
make

comment:9 Changed 2 years ago by mderickx

  • Status changed from new to needs_info

comment:10 Changed 2 years ago by mderickx

  • 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 --with-mp=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.

Last edited 2 years ago by mderickx (previous) (diff)

Changed 2 years ago by mderickx

comment:11 Changed 2 years ago by mderickx

The commands:

make configure
./configure --with-mp=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.

Last edited 2 years ago by mderickx (previous) (diff)

comment:12 Changed 2 years ago by fbissey

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 2 years ago by fbissey

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 2 years ago by mderickx

It took me longer then it should 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.

Last edited 2 years ago by mderickx (previous) (diff)

comment:15 Changed 2 years ago by fbissey

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 2 years ago by jdemeyer

Your patch makes sense, but you will need to special-case 32-bit. I'm pretty sure that the 32-bit result is an OverflowError both with mpir and with gmp.

comment:17 Changed 2 years ago by mderickx

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 2 years ago by jdemeyer

The OverflowError is raised by Cython, so it would be the same for mpir and gmp.

comment:19 Changed 2 years ago by git

  • Commit changed from beef1a813eecf7d24de8f05ec839f824dc5275ee to dcb984a4f99eefbfbd5064bf82fda815482c601d

Branch pushed to git repo; I updated commit sha1. New commits:

dcb984aTrac 23913: Fixed doctest failures with GMP in 32-bit

comment:20 Changed 2 years ago by mderickx

  • Status changed from needs_work to needs_review

Ok, I tested the current branch on:

  • 32-bit gmp + #19706
  • 32-bit mpir
  • 64-bit mpir

I still need to test 64-bit 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 2 years ago by mderickx

  • Branch changed from public/23913 to public/23913_clean
  • Commit changed from dcb984a4f99eefbfbd5064bf82fda815482c601d to bf7056499f7e4c9a7fca342523238a27144cc8c9

Ok the case: 64-bit 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/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/jdemeyer/sage/local/lib/python2.7/site-packages/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:

bf70564Trac #23913: Fixed doctest failures with GMP

comment:22 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:23 Changed 2 years ago by vbraun

  • Branch changed from public/23913_clean to bf7056499f7e4c9a7fca342523238a27144cc8c9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.