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:  sage6.4 
Component:  packages: standard  Keywords:  
Cc:  fbissey  Merged in:  
Authors:  JeanPierre Flori  Reviewers:  François Bissey 
Report Upstream:  N/A  Work issues:  
Branch:  910ed7a (Commits, GitHub, GitLab)  Commit:  910ed7a62addb3f01f0afa2b58180a3ca0639cc1 
Dependencies:  Stopgaps: 
Description (last modified by )
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
 Branch set to u/jpflori/ticket/15957
 Cc fbissey added
 Commit set to 64802d4fd81be99903abe87c2c77261cef77808c
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:3 Changed 7 years ago by
 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^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/marc/co/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/home/marc/co/sage/local/lib/python2.7/sitepackages/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, aconfigure
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
 Status changed from needs_review to needs_work
comment:5 followup: ↓ 6 Changed 7 years ago by
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 ; followup: ↓ 7 Changed 7 years ago by
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^632) ....: 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
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
Also note GMP 6.0.0 is out.
comment:9 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 6 years ago by
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
Indeed I only have the memory issue left in sageongentoo. I'll review anything you put here.
comment:12 Changed 6 years ago by
 Commit changed from 64802d4fd81be99903abe87c2c77261cef77808c to 914b36d6d403047c38e368addc263433d8fd315b
comment:13 Changed 6 years ago by
 Commit changed from 914b36d6d403047c38e368addc263433d8fd315b to 24ba1730bc28e48848ce979d98c274b439a59358
Branch pushed to git repo; I updated commit sha1. New commits:
24ba173  Add documentation for SAGE_MP_LIBRARY.

comment:14 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:15 Changed 6 years ago by
 Commit changed from 24ba1730bc28e48848ce979d98c274b439a59358 to b4adf9647e7b23671fe48d1a04fc3a06508f10c4
Branch pushed to git repo; I updated commit sha1. New commits:
b4adf96  Add a little more info for SAGE_MP_LIBRARY.

comment:16 Changed 6 years ago by
 Reviewers set to François Bissey
 Status changed from needs_review to positive_review
Looks good to me.
comment:17 followup: ↓ 23 Changed 6 years ago by
 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 sagespkg 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
Oops, I'll quickly fix that.
comment:19 Changed 6 years ago by
 Commit changed from b4adf9647e7b23671fe48d1a04fc3a06508f10c4 to 910ed7a62addb3f01f0afa2b58180a3ca0639cc1
Branch pushed to git repo; I updated commit sha1. New commits:
910ed7a  Remove SAGE_UPGRADING related code.

comment:20 Changed 6 years ago by
Looks better now.
comment:21 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:23 in reply to: ↑ 17 Changed 6 years ago by
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 sagespkg with # "f" to force rebuilding dependent packages, too: export SAGE_SPKG_OPTS="f" fishould 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
 Branch changed from u/jpflori/ticket/15957 to 910ed7a62addb3f01f0afa2b58180a3ca0639cc1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Initial commit for GMP 5.1.3.
Explicit conversion in farey.cpp as recent GMP do not do it automatically.
USe GMP or MPIR as default MP library.
Merge remotetracking branch 'trac/develop' into ticket/15957
Introduce variables to switch between MPIR and GMP.