Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3: cysignals fails its test suite #28726

Closed
jhpalmieri opened this issue Nov 13, 2019 · 20 comments
Closed

py3: cysignals fails its test suite #28726

jhpalmieri opened this issue Nov 13, 2019 · 20 comments

Comments

@jhpalmieri
Copy link
Member

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

CC: @jdemeyer

Component: packages: standard

Author: John Palmieri

Branch/Commit: f4f3c86

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/28726

@jhpalmieri
Copy link
Member Author

comment:1

The change

diff --git a/build/pkgs/cysignals/spkg-check b/build/pkgs/cysignals/spkg-check
index c94041e096..37fa0dddfc 100644
--- a/build/pkgs/cysignals/spkg-check
+++ b/build/pkgs/cysignals/spkg-check
@@ -4,4 +4,4 @@ if [ -z "$SAGE_LOCAL" ]; then
     exit 1
 fi
 
-cd src && $MAKE check-install
+cd 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

@vbraun
Copy link
Member

vbraun commented Nov 14, 2019

comment:2

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?)

@vbraun
Copy link
Member

vbraun commented Nov 15, 2019

comment:4

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

@jhpalmieri
Copy link
Member Author

@jhpalmieri
Copy link
Member Author

Commit: f4f3c86

@jhpalmieri
Copy link
Member Author

comment:6

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:

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

New commits:

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

@jhpalmieri
Copy link
Member Author

comment:7

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.

@jhpalmieri
Copy link
Member Author

comment:8

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?

@vbraun
Copy link
Member

vbraun commented Nov 15, 2019

comment:9

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)

@jhpalmieri
Copy link
Member Author

comment:10

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.)

@jhpalmieri
Copy link
Member Author

comment:11
$ 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.

@jhpalmieri
Copy link
Member Author

comment:12

Is this a recurrence of sagemath/cysignals#71?

@jhpalmieri
Copy link
Member Author

comment:13

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

@jhpalmieri
Copy link
Member Author

comment:14

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

@vbraun
Copy link
Member

vbraun commented Nov 15, 2019

comment:15

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...

@vbraun
Copy link
Member

vbraun commented Nov 15, 2019

Author: John Palmieri

@vbraun
Copy link
Member

vbraun commented Nov 15, 2019

Reviewer: Volker Braun

@jhpalmieri
Copy link
Member Author

comment:17

Okay, I opened up a cysignals issue.

@vbraun
Copy link
Member

vbraun commented Nov 16, 2019

comment:18

Thanks! Bug is at sagemath/cysignals#118 for future historians ;-)

@vbraun
Copy link
Member

vbraun commented Nov 16, 2019

Changed branch from u/jhpalmieri/cysignals-python-version to f4f3c86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants