Opened 7 years ago

Closed 6 years ago

#15957 closed enhancement (fixed)

Let easily switch between GMP and MPIR

Reported by: jpflori Owned by:
Priority: major Milestone: sage-6.4
Component: packages: standard Keywords:
Cc: fbissey Merged in:
Authors: Jean-Pierre Flori Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 910ed7a (Commits) Commit: 910ed7a62addb3f01f0afa2b58180a3ca0639cc1
Dependencies: Stopgaps:

Description (last modified by jpflori)

Follow up to #12661.

One can set SAGE_MP_LIBRARY to choose between GMP and MPIR when issueing make.

Change History (24)

comment:1 Changed 7 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • Branch set to u/jpflori/ticket/15957
  • Cc fbissey added
  • Commit set to 64802d4fd81be99903abe87c2c77261cef77808c
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

2c34755Initial commit for GMP 5.1.3.
a14aaf2Explicit conversion in farey.cpp as recent GMP do not do it automatically.
a3bf5a1USe GMP or MPIR as default MP library.
aa61de8Merge remote-tracking branch 'trac/develop' into ticket/15957
64802d4Introduce variables to switch between MPIR and GMP.

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 7 years ago by mmezzarobba

  • I get a few failures when running the tests after building sage with SAGE_MP_LIBRARY=GMP:
    sage -t src/sage/rings/integer.pyx
    **********************************************************************
    File "src/sage/rings/integer.pyx", line 1959, in sage.rings.integer.Integer.__pow__
    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/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
            self.execute(example, compiled, test.globs)
          File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 839, in execute
            exec compiled in globs
          File "<doctest sage.rings.integer.Integer.__pow__[27]>", line 1, in <module>
            Integer(2)**(Integer(2)**Integer(63)-Integer(2))
          File "integer.pyx", line 1998, in sage.rings.integer.Integer.__pow__ (sage/rings/integer.c:14703)
          File "c_lib.pyx", line 85, in sage.ext.c_lib.sig_raise_exception (sage/ext/c_lib.c:1040)
        RuntimeError: Aborted
    **********************************************************************
    File "src/sage/rings/integer.pyx", line 5431, in sage.rings.integer.Integer._xgcd
    Failed example:
        2._xgcd(-2)
    Expected:
        (2, 1, 0)
    Got:
        (2, 0, -1)
    **********************************************************************
    
  • The new environment variable should probably be documented in src/doc/en/installation/source.rst. (Well, in the long run, a configure flag would IMO be much more appropriate than an environment variable.)
  • The GMP tarball apparently still needs to be mirrored (see #12661).

comment:4 Changed 7 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

comment:5 follow-up: Changed 7 years ago by fbissey

You forgot one. But those are known from #12661. I am not sure we can fix minor differences in behavior. We can choose different tests in some case but the expected failure will be hard to replace.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by mmezzarobba

Replying to fbissey:

You forgot one. But those are known from #12661.

You are completely right, thanks!

I am not sure we can fix minor differences in behavior.

Ok, I still don't like keeping around doctests that are know to fail, but I don't want to let the issue block this ticket.

We can choose different tests in some case but the expected failure will be hard to replace.

What about something like

sage: try: 2^(2^63-2)
....: except MemoryError, RuntimeError: true

?

And does anyone understand if the output of the failing test in sage/tests/book_stein_modform.py is correct?

comment:7 in reply to: ↑ 6 Changed 7 years ago by jpflori

Replying to mmezzarobba:

And does anyone understand if the output of the failing test in sage/tests/book_stein_modform.py is correct?

I don't, but from the look of it, it seems plausible it is still correct :)

comment:8 Changed 7 years ago by jpflori

Also note GMP 6.0.0 is out.

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 6 years ago by jpflori

I guess the doctests issues (except for the memory one) are gone now that MPIR behaves just like GMP, see #15015.

If I rebase the ticket, would anyone be kind enough to review it? Maybe I'll just simplify the ticket discarding the black magic to be able to change the MP lib you use. Then you just choose which one you use the first time you build Sage and live with it. Just like with SAGE_DEBUG.

comment:11 Changed 6 years ago by fbissey

Indeed I only have the memory issue left in sage-on-gentoo. I'll review anything you put here.

comment:12 Changed 6 years ago by git

  • Commit changed from 64802d4fd81be99903abe87c2c77261cef77808c to 914b36d6d403047c38e368addc263433d8fd315b

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

d1eef69Merge remote-tracking branch 'trac/develop' into ticket/15957
914b36dRemove possibility to switch from GMP to MPIR without building from scratch.

comment:13 Changed 6 years ago by git

  • Commit changed from 914b36d6d403047c38e368addc263433d8fd315b to 24ba1730bc28e48848ce979d98c274b439a59358

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

24ba173Add documentation for SAGE_MP_LIBRARY.

comment:14 Changed 6 years ago by jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:15 Changed 6 years ago by git

  • Commit changed from 24ba1730bc28e48848ce979d98c274b439a59358 to b4adf9647e7b23671fe48d1a04fc3a06508f10c4

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

b4adf96Add a little more info for SAGE_MP_LIBRARY.

comment:16 Changed 6 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Looks good to me.

comment:17 follow-up: Changed 6 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

I don't think the lines

###############################################################################
# Determine whether we should automatically rebuild dependencies.
###############################################################################

if [ "$SAGE_UPGRADING" = yes ]; then
 # We're doing an upgrade. Let build/Makefile call sage-spkg with
 # "-f" to force rebuilding dependent packages, too:
 export SAGE_SPKG_OPTS="-f"
fi

should be there: I think they are a remnant from an older version of Sage. The environment variable SAGE_UPGRADING was also removed. See #17072.

comment:18 Changed 6 years ago by jpflori

Oops, I'll quickly fix that.

comment:19 Changed 6 years ago by git

  • Commit changed from b4adf9647e7b23671fe48d1a04fc3a06508f10c4 to 910ed7a62addb3f01f0afa2b58180a3ca0639cc1

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

910ed7aRemove SAGE_UPGRADING related code.

comment:20 Changed 6 years ago by jpflori

Looks better now.

comment:21 Changed 6 years ago by jpflori

  • Status changed from needs_work to needs_review

comment:22 Changed 6 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Okay, thanks.

comment:23 in reply to: ↑ 17 Changed 6 years ago by fbissey

Replying to jhpalmieri:

I don't think the lines

###############################################################################
# Determine whether we should automatically rebuild dependencies.
###############################################################################

if [ "$SAGE_UPGRADING" = yes ]; then
 # We're doing an upgrade. Let build/Makefile call sage-spkg with
 # "-f" to force rebuilding dependent packages, too:
 export SAGE_SPKG_OPTS="-f"
fi

should be there: I think they are a remnant from an older version of Sage. The environment variable SAGE_UPGRADING was also removed. See #17072.

It does look like a left over, yes.

comment:24 Changed 6 years ago by vbraun

  • Branch changed from u/jpflori/ticket/15957 to 910ed7a62addb3f01f0afa2b58180a3ca0639cc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.