Opened 5 years ago

Closed 5 years ago

#23957 closed defect (fixed)

gmpy2 causes doctest failures

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.1
Component: packages: optional Keywords:
Cc: vdelecroix, mderickx Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix, Maarten Derickx
Report Upstream: N/A Work issues:
Branch: fea3848 (Commits, GitHub, GitLab) Commit: fea3848d0d397a7abbc196b3de93df48a2b0fbc3
Dependencies: Stopgaps:

Status badges

Description

The current version of gmpy2 has issues, in particular doctest failures like

sage -t 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:
    Fatal Python error: Insufficient memory
    Traceback (most recent call last):
      File "/home/worker/sage-patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/worker/sage-patchbot/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
**********************************************************************

Change History (15)

comment:1 Changed 5 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Cc mderickx added
  • Component changed from packages: experimental to packages: optional
  • Priority changed from major to blocker

comment:2 Changed 5 years ago by vdelecroix

Why is gmpy2 involved here!?

comment:3 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/gmpy2_causes_doctest_failures

comment:4 Changed 5 years ago by jdemeyer

  • Commit set to fea3848d0d397a7abbc196b3de93df48a2b0fbc3
  • Status changed from new to needs_review

New commits:

fea3848Do not call mp_set_memory_functions() in gmpy2

comment:5 follow-up: Changed 5 years ago by mderickx

Is this independent of whether gmp or mpir is used?

comment:6 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to mderickx:

Is this independent of whether gmp or mpir is used?

What do you mean with "this"?

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

I mean the doctest failure, is your install using gmp or mpir. Or is this something that does not matter?

comment:8 in reply to: ↑ 7 Changed 5 years ago by jdemeyer

Replying to mderickx:

I mean the doctest failure, is your install using gmp or mpir.

I'm using mpir, but the failure is caused by gmpy2. So I still don't really understand your question.

comment:9 Changed 5 years ago by vdelecroix

Luckily, this is also fixed in gmpy2 version 2.X

comment:10 Changed 5 years ago by mderickx

Well gmpy2 is a python interface that can use both mpir or gmp as back end. And we already know that 2^(2^63-2) causes different errors depending on wether one uses gmp or mpir. I dont know much about the inner workings of gmpy2 so maybe my question does not make sense to you because you know more about the inner workings of gmpy2 then I do, but with my current knowledge I wouldn't be surprised if memory management works differently depending whether mpir or gmp is used as backend, and this therefore deserves testing both with mpir and gmp.

comment:11 Changed 5 years ago by vdelecroix

I don't see the point of testing this on GMP (though it would not hurt). gmpy2 was "wrongly" setting a custom allocation scheme in GMP (or MPIR). Moreover the fix proposed is the way it is in development versions of gmpy2.

comment:12 Changed 5 years ago by mderickx

Note that without this ticked, but with gmp and gmpy2 there is just the expected gmp error:

bt-nac-b021:sage_gmp mderickx$ ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 8.1.beta7, Release Date: 2017-10-03               │
│ Type "notebook()" for the browser-based notebook interface.        │
│ Type "help()" for help.                                            │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
Forcing sage-location, probably because a new package was installed.
Cleaning up, do not interrupt this.
Done cleaning.
sage: 2^(2^63-2)
gmp: overflow in mpz type
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-1-b138756ea9ad> in <module>()
----> 1 Integer(2)**(Integer(2)**Integer(63)-Integer(2))

/Applications/sage_gmp/src/sage/rings/integer.pyx in sage.rings.integer.Integer.__pow__ (build/cythonized/sage/rings/integer.c:14250)()
   2065         cdef Integer x = PY_NEW(Integer)
   2066 
-> 2067         sig_on()
   2068         mpz_pow_ui(x.value, (<Integer>self).value, nn if nn > 0 else -nn)
   2069         sig_off()

RuntimeError: Aborted
sage: optional_packages()[0]
['ccache', 'gmp', 'gmpy2', 'python2']

I do get the error reported in this ticket with just mpir. So there is difference between using gmp and mpir. I am now testing everything with this ticket applied

comment:13 Changed 5 years ago by mderickx

Ok this ticket passes the 2^(2^63-2) test both with mpir and gmp for my on OS X 0.12.4 . I do not understand the exact change and its ramification, but it seems that Vincent does so I leave it up to him to give positive review.

comment:14 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix, Maarten Derickx
  • Status changed from needs_review to positive_review

GMP/MPIR allows for custom memory allocation function (in replacement for malloc/realloc/free). gmpy2 was changing them by default which is not a reasonable policy.

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/gmpy2_causes_doctest_failures to fea3848d0d397a7abbc196b3de93df48a2b0fbc3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.