#28726 closed defect (fixed)

py3: cysignals fails its test suite

Reported by: jhpalmieri Owned by:
Priority: critical Milestone: sage-9.0
Component: packages: standard Keywords:
Cc: jdemeyer Merged in:
Authors: John Palmieri Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: f4f3c86 (Commits, GitHub, GitLab) Commit: f4f3c8630d9b9f868853fafd8b5e6cc9eb785ff4
Dependencies: Stopgaps:

Status badges

Description

With a Python 3 build of Sage, ./sage -f -c cysignals fails because the test suite doesn't use the correct version of Python.

Change History (19)

comment:1 Changed 23 months ago by jhpalmieri

The change

  • build/pkgs/cysignals/spkg-check

    diff --git a/build/pkgs/cysignals/spkg-check b/build/pkgs/cysignals/spkg-check
    index c94041e096..37fa0dddfc 100644
    a b if [ -z "$SAGE_LOCAL" ]; then 
    44    exit 1
    55fi
    66
    7 cd src && $MAKE check-install
     7cd src && $MAKE check-install PYTHON=sage-python23

helps testing get started, but the test suite still fails on OS X. I see this message in the log file:

Traceback (most recent call last):
  File "rundoctests.py", line 74, in <module>
    resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
ValueError: current limit exceeds maximum limit

comment:2 Changed 23 months ago by vbraun

I'm seeing that one, too. Only admins can increase the stack size, there is nothing that the testsuite can do about it (except not running the test, does it really need that big a stack?)

comment:3 Changed 23 months ago by jhpalmieri

  • Cc jdemeyer added

comment:4 Changed 23 months ago by vbraun

Maybe you can turn your diff already into a patch, we should surely be using the correct python version.

comment:5 Changed 23 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/cysignals-python-version

comment:6 Changed 23 months ago by jhpalmieri

  • Commit set to f4f3c8630d9b9f868853fafd8b5e6cc9eb785ff4

I don't understand the stack size issue. The following patch is a bad idea since it completely disables the stack size limitation put in by cysignals' rundoctests.py, but with it, the test suite passes for me with OS X + py3:

  • new file uild/pkgs/cysignals/patches/stacksize.patch

    diff --git a/build/pkgs/cysignals/patches/stacksize.patch b/build/pkgs/cysignals/patches/stacksize.patch
    new file mode 100644
    index 0000000000..63a11ba07f
    - +  
     1diff --git a/rundoctests.py b/rundoctests.py
     2index 94d58e1..3c4f8c3 100755
     3--- a/rundoctests.py
     4+++ b/rundoctests.py
     5@@ -71,7 +71,7 @@ if os.name != 'nt':
     6     import resource
     7     # Limit stack size to avoid errors in stack overflow doctest
     8     stacksize = 1 << 20
     9-    resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
     10+    # resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
     11 
     12     # Disable core dumps
     13     resource.setrlimit(resource.RLIMIT_CORE, (0, 0))

New commits:

f4f3c86trac 28726: use the correct version of Python in cysignals spkg-check

comment:7 Changed 23 months ago by jhpalmieri

I'm not marking this ready for review, because even with the patch specifying the correction version of Python, it doesn't pass its test suite.

comment:8 Changed 23 months ago by jhpalmieri

On the other hand, with just this patch, tests do pass on my ubuntu virtual machine. So maybe the stack size issue is OS X only?

comment:9 Changed 23 months ago by vbraun

So we are trying to reduce the stack size, not increase it. 1MB is pretty low. Whats the output of ulimit -s on your OSX machine? I think its 8192 (8MB) by default, which should be plenty. I don't get an error running:

OSX:~ vbraun$ python
Python 2.7.10 (default, Feb 22 2019, 21:55:15) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import resource
>>> resource.getrlimit(resource.RLIMIT_STACK)
(8388608, 67104768)
>>> stacksize = 1 << 20
>>> resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
>>> resource.getrlimit(resource.RLIMIT_STACK)
(1048576, 1048576)
Last edited 23 months ago by vbraun (previous) (diff)

comment:10 Changed 23 months ago by jhpalmieri

ulimit -s returns 8192. I don't get an error running that with Python 2, but I do with Python 3 on OS X. (On the Ubuntu virtual machine, I don't get an error with Python 3.)

comment:11 Changed 23 months ago by jhpalmieri

$ python
Python 2.7.16 (default, Oct 16 2019, 00:34:56) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import resource
>>> resource.getrlimit(resource.RLIMIT_STACK)
(8388608, 67104768)
>>> stacksize = 1 << 20
>>> resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
>>> resource.getrlimit(resource.RLIMIT_STACK)
(1048576, 1048576)

and

$ sage --python
Python 3.7.3 (default, Nov  5 2019, 10:19:31) 
[Clang 11.0.0 (clang-1100.0.33.12)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import resource
>>> resource.getrlimit(resource.RLIMIT_STACK)
(8388608, 67104768)
>>> stacksize = 1 << 20
>>> resource.setrlimit(resource.RLIMIT_STACK, (stacksize, stacksize))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: current limit exceeds maximum limit

I get the same error if I use python3, but I forget how I installed Python 3 on this machine.

comment:12 Changed 23 months ago by jhpalmieri

comment:13 Changed 23 months ago by jhpalmieri

Or maybe it's https://bugs.python.org/issue34602, and upgrading Python will help.

comment:14 Changed 23 months ago by jhpalmieri

I installed Python 3.7.5, and I still hit this problem.

comment:15 Changed 23 months ago by vbraun

  • Authors set to John Palmieri
  • Reviewers set to Volker Braun
  • Status changed from new to needs_review

Do you mind opening a cysignals issue for the OSX kernel oddity / bug? The ValueError should probably just be caught and discarded. But we could merge this in the meantime...

comment:16 Changed 23 months ago by vbraun

  • Status changed from needs_review to positive_review

comment:17 Changed 23 months ago by jhpalmieri

Okay, I opened up a cysignals issue.

comment:18 Changed 23 months ago by vbraun

Thanks! Bug is at https://github.com/sagemath/cysignals/issues/118 for future historians ;-)

comment:19 Changed 23 months ago by vbraun

  • Branch changed from u/jhpalmieri/cysignals-python-version to f4f3c8630d9b9f868853fafd8b5e6cc9eb785ff4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.