remaining open issues for 0.17
Hi, please correct me if I'm missing something, but besides the fused types default value thing, I currently see two open issues for 0.17. One is the 32bit problem we have in the buffer code, the other one is the "vile hack" that Robert added as a type inference fix. I just ran into the following code (while compiling Django) which currently crashes the compiler, and I think it's due to one of those "type inference modifies state" issues. """ def func(**kwargs): """ >>> sorted(func(a=3, b=4)) [1, 2, 3, 4] """ return [ arg for arg in [1,2] + kwargs.values() ] """ How should we deal with these? Personally, I consider general C code portability issues more annoying than code that fails to compile but that can be worked around by users. So, unless we can fix the default values and/or the type inference issues soonishly, I would concentrate on the 32bit issues for now and release 0.17 when we get them resolved. Thoughts? Stefan
On 6 August 2012 22:58, Stefan Behnel <stefan_ml@behnel.de> wrote:
Hi,
please correct me if I'm missing something, but besides the fused types default value thing, I currently see two open issues for 0.17. One is the 32bit problem we have in the buffer code, the other one is the "vile hack" that Robert added as a type inference fix. I just ran into the following code (while compiling Django) which currently crashes the compiler, and I think it's due to one of those "type inference modifies state" issues.
""" def func(**kwargs): """ >>> sorted(func(a=3, b=4)) [1, 2, 3, 4] """ return [ arg for arg in [1,2] + kwargs.values() ] """
How should we deal with these? Personally, I consider general C code portability issues more annoying than code that fails to compile but that can be worked around by users. So, unless we can fix the default values and/or the type inference issues soonishly, I would concentrate on the 32bit issues for now and release 0.17 when we get them resolved.
Thoughts?
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta. Stefan
On Thu, 09 Aug 2012, Stefan Behnel wrote:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
There might have been a misunderstanding -- I can't recall if I have stated that 32-bit issues were not fixed (but I can be wrong). Probably you meant my reply concerning s390x builds (below)... and I have not uploaded any new revision to Debian since 0.17 beta1. ,--- | Date: Thu, 2 Aug 2012 10:30:32 -0400 | From: Yaroslav Halchenko <lists@onerussian.com> | To: cython-devel@python.org | Subject: Re: [Cython] [cython-users] Re: Cython 0.17 beta 1 released | | | On Wed, 01 Aug 2012, mark florisson wrote: | > Thanks for the fix. I also pushed a fix for one more test numpy_test | > related to fused types dispatching. That passes all tests for me on 32 | > bit linux. | > > Yaroslav, could you give it a try on the Debian build servers? | | FWIW -- 0.16rc1-550-g8880c78 hasn't added nor fixed any of the failures | on s390x in comparison to 0.16rc1-547-g68811fa | ;-) | `--- -- Yaroslav O. Halchenko Postdoctoral Fellow, Department of Psychological and Brain Sciences Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik
FWIW -- just double checked -- tests pass (of current master) just fine on an i386 Debian system. On Thu, 09 Aug 2012, Yaroslav Halchenko wrote:
On Thu, 09 Aug 2012, Stefan Behnel wrote:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
There might have been a misunderstanding -- I can't recall if I have stated that 32-bit issues were not fixed (but I can be wrong). Probably you meant my reply concerning s390x builds (below)... and I have not uploaded any new revision to Debian since 0.17 beta1.
,--- | Date: Thu, 2 Aug 2012 10:30:32 -0400 | From: Yaroslav Halchenko <lists@onerussian.com> | To: cython-devel@python.org | Subject: Re: [Cython] [cython-users] Re: Cython 0.17 beta 1 released
| On Wed, 01 Aug 2012, mark florisson wrote: | > Thanks for the fix. I also pushed a fix for one more test numpy_test | > related to fused types dispatching. That passes all tests for me on 32 | > bit linux. | > > Yaroslav, could you give it a try on the Debian build servers?
| FWIW -- 0.16rc1-550-g8880c78 hasn't added nor fixed any of the failures | on s390x in comparison to 0.16rc1-547-g68811fa | ;-)
`--- -- Yaroslav O. Halchenko Postdoctoral Fellow, Department of Psychological and Brain Sciences Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
From a quick look, they might really be problems in the tests rather than
He ran them through the build servers. Here are the latest results: https://buildd.debian.org/status/package.php?p=cython&suite=experimental (hint: click on "all" in the Logs column, then click on the build result to see the log output) The i386 tests show 4 errors related to fused types in the NumPy tests: https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255 the code we generate. It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test: https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358 It now prints """ ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """ Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ... To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error. Stefan
On 9 August 2012 16:36, Stefan Behnel <stefan_ml@behnel.de> wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does if (type->is_unsigned) *buf = toupper(*buf); So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me. Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though. Is it bad to release something that doesn't pass the entire test suite on some system?
Stefan
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
mark florisson, 09.08.2012 18:51:
On 9 August 2012 16:36, Stefan Behnel wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does
if (type->is_unsigned) *buf = toupper(*buf);
So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me.
I think this needs some more investigation directly on the failing systems. It'll be much easier to figure this out with a debugger.
Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though.
If we can't find a way to fix this in time, I'm leaning towards that, too. However, working around a bug in a test is a sure way to forget about the bug and consider everything working. Could you copy the failing tests into a separate test case, disable that in bugs.txt and then change the types in the original test modules to whatever you think would make the tests work?
Is it bad to release something that doesn't pass the entire test suite on some system?
Given that we already made tons of releases without even knowing that they'd fail on some systems, I'd say no. :) So far, we've always taken care of the most common systems and just tried to accommodate for other systems to a certain extent. It really doesn't matter if we fix these things before or after the 0.17 release. Stefan
On Thu, 09 Aug 2012, Stefan Behnel wrote:
Is it bad to release something that doesn't pass the entire test suite on some system?
Given that we already made tons of releases without even knowing that they'd fail on some systems, I'd say no. :)
FWIW -- some time ago 0.15.1 build/passed all the tests across all architectures. https://buildd.debian.org/status/package.php?p=cython&suite=sid -- Yaroslav O. Halchenko Postdoctoral Fellow, Department of Psychological and Brain Sciences Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755 Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419 WWW: http://www.linkedin.com/in/yarik
On 9 August 2012 18:04, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 09.08.2012 18:51:
On 9 August 2012 16:36, Stefan Behnel wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does
if (type->is_unsigned) *buf = toupper(*buf);
So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me.
I think this needs some more investigation directly on the failing systems. It'll be much easier to figure this out with a debugger.
Definitely. Do we have access to those kind of machines, though?
Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though.
If we can't find a way to fix this in time, I'm leaning towards that, too. However, working around a bug in a test is a sure way to forget about the bug and consider everything working. Could you copy the failing tests into a separate test case, disable that in bugs.txt and then change the types in the original test modules to whatever you think would make the tests work?
I think the same bug is failing all these cases. I think the thing that's really failing is this: char[:] m = <char[:N]> my_char_pointer
Is it bad to release something that doesn't pass the entire test suite on some system?
Given that we already made tons of releases without even knowing that they'd fail on some systems, I'd say no. :)
So far, we've always taken care of the most common systems and just tried to accommodate for other systems to a certain extent. It really doesn't matter if we fix these things before or after the 0.17 release.
Great :) I'm sorry, but I'm really swamped for the next two weeks (dissertation deadline), so I'm thinking of just leaving these few test failures, they're not really worrisome.
Stefan
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
On Thu, Aug 9, 2012 at 10:04 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 09.08.2012 18:51:
On 9 August 2012 16:36, Stefan Behnel wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does
if (type->is_unsigned) *buf = toupper(*buf);
So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me.
I think this needs some more investigation directly on the failing systems. It'll be much easier to figure this out with a debugger.
Strangely enough, I wasn't even able to reproduce this passing "-funsigned-char" so yes, I think it may require access to one of these systems. (Perhaps I'd need to compile Python and/or NumPy with this option as well...)
Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though.
If we can't find a way to fix this in time, I'm leaning towards that, too. However, working around a bug in a test is a sure way to forget about the bug and consider everything working. Could you copy the failing tests into a separate test case, disable that in bugs.txt and then change the types in the original test modules to whatever you think would make the tests work?
I'll take a stab at this.
Is it bad to release something that doesn't pass the entire test suite on some system?
Given that we already made tons of releases without even knowing that they'd fail on some systems, I'd say no. :)
So far, we've always taken care of the most common systems and just tried to accommodate for other systems to a certain extent. It really doesn't matter if we fix these things before or after the 0.17 release.
+1 - Robert
On Fri, Aug 10, 2012 at 9:25 AM, Robert Bradshaw <robertwb@gmail.com> wrote:
On Thu, Aug 9, 2012 at 10:04 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 09.08.2012 18:51:
On 9 August 2012 16:36, Stefan Behnel wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09:
I thought the 32 bit issue was resolved? You pushed a fix and I fixed some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does
if (type->is_unsigned) *buf = toupper(*buf);
So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me.
I think this needs some more investigation directly on the failing systems. It'll be much easier to figure this out with a debugger.
Strangely enough, I wasn't even able to reproduce this passing "-funsigned-char" so yes, I think it may require access to one of these systems. (Perhaps I'd need to compile Python and/or NumPy with this option as well...)
Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though.
If we can't find a way to fix this in time, I'm leaning towards that, too. However, working around a bug in a test is a sure way to forget about the bug and consider everything working. Could you copy the failing tests into a separate test case, disable that in bugs.txt and then change the types in the original test modules to whatever you think would make the tests work?
I'll take a stab at this.
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html . I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct. It'd be good to see if this actually fixes the issues. - Robert
Robert Bradshaw, 10.08.2012 20:54:
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html . I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct.
That's unfortunate, but since it's a bug, it needs fixing. Stefan
On Fri, Aug 10, 2012 at 12:10 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Robert Bradshaw, 10.08.2012 20:54:
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html . I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct.
That's unfortunate, but since it's a bug, it needs fixing.
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh. - Robert
On Fri, Aug 10, 2012 at 12:31 PM, Robert Bradshaw <robertwb@gmail.com> wrote:
On Fri, Aug 10, 2012 at 12:10 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Robert Bradshaw, 10.08.2012 20:54:
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html . I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct.
That's unfortunate, but since it's a bug, it needs fixing.
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
I suppose the lack of numpy support for (no-signed) char does explain why we were in the state we were in. I've pushed another commit that lets unsigned char == char == signed char for the purposes of buffer comparison (though not transitively, so unsigned char != signed char). - Robert
Robert Bradshaw, 10.08.2012 22:07:
On Fri, Aug 10, 2012 at 12:31 PM, Robert Bradshaw wrote:
On Fri, Aug 10, 2012 at 12:10 PM, Stefan Behnel wrote:
Robert Bradshaw, 10.08.2012 20:54:
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html . I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct.
That's unfortunate, but since it's a bug, it needs fixing.
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
Many people only run their code on one particular platform (usually some x86 flavour), often just on their own machine. Disallowing it all together sounds overly extreme in that light.
I suppose the lack of numpy support for (no-signed) char does explain why we were in the state we were in. I've pushed another commit that lets unsigned char == char == signed char for the purposes of buffer comparison (though not transitively, so unsigned char != signed char).
I find that behaviour acceptable. "char" is underdefined in C and the only really safe value range is 0..127. So we can leave it to users to get it right if they really want to use it. Stefan
I think again this is an instance of the tests being incorrect, rather than the code. The failing numpy_tests are all cases where we instantiate a buffer of type 'b' (== 'signed char') but then try to use it as if it was a 'char'. The fix, of course is to just replace most instances of 'char' with 'signed char' in numpy_test.pyx: https://github.com/bfroehle/cython/compare/char-buffers In some sense NumPy does have a notion of 'char', namely a 1 character string. The only worry here, of course, is whether this change will painfully break existing code… -Brad
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
On 10 August 2012 21:27, Bradley M. Froehle <brad.froehle@gmail.com> wrote:
I think again this is an instance of the tests being incorrect, rather than the code. The failing numpy_tests are all cases where we instantiate a buffer of type 'b' (== 'signed char') but then try to use it as if it was a 'char'.
Yes, those tests are definitely wrong. I'm still wondering about the case where we don't write the format string ourselves, but have it generated, though.
The fix, of course is to just replace most instances of 'char' with 'signed char' in numpy_test.pyx: https://github.com/bfroehle/cython/compare/char-buffers
In some sense NumPy does have a notion of 'char', namely a 1 character string.
The only worry here, of course, is whether this change will painfully break existing code…
-Brad
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
I agree. Non-transitive type comparisons seems like very fishy business (it will be *very* surprising to whoever runs across it); I think there's a strong case for just breaking backwards compatability: ERROR: 'char' is illegal as a buffer dtype due to being underspecified in the C standard, please use 'signed char' or 'unsigned char'. (Myself I always use uint8_t or int8_t rather than char...) Dag -- Sent from my Android phone with K-9 Mail. Please excuse my brevity. "Bradley M. Froehle" <brad.froehle@gmail.com> wrote: I think again this is an instance of the tests being incorrect, rather than the code. The failing numpy_tests are all cases where we instantiate a buffer of type 'b' (== 'signed char') but then try to use it as if it was a 'char'. The fix, of course is to just replace most instances of 'char' with 'signed char' in numpy_test.pyx: https://github.com/bfroehle/cython/compare/char-buffers In some sense NumPy does have a notion of 'char', namely a 1 character string. The only worry here, of course, is whether this change will painfully break existing code… -Brad Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
We have plenty of non-transitive coercions. E.g. char* <-> object <-> float. While it's technically more correct to use signed or unsigned char, char* is pretty ubiquitous in the C world. There's also the question of the python bytes object and array.array("c") which would be good to support. And this would disallow any nested chars as parts of structs (e.g. from external libraries--you could fake a signness in your declaration but that seems ugly). All in all, particularly in light of interfacing with external code, I think disallowing char is overly strict. On Fri, Aug 10, 2012 at 1:38 PM, Dag Sverre Seljebotn <d.s.seljebotn@astro.uio.no> wrote:
I agree. Non-transitive type comparisons seems like very fishy business (it will be *very* surprising to whoever runs across it); I think there's a strong case for just breaking backwards compatability:
ERROR: 'char' is illegal as a buffer dtype due to being underspecified in the C standard, please use 'signed char' or 'unsigned char'.
(Myself I always use uint8_t or int8_t rather than char...)
Dag -- Sent from my Android phone with K-9 Mail. Please excuse my brevity.
"Bradley M. Froehle" <brad.froehle@gmail.com> wrote:
I think again this is an instance of the tests being incorrect, rather than the code. The failing numpy_tests are all cases where we instantiate a buffer of type 'b' (== 'signed char') but then try to use it as if it was a 'char'.
The fix, of course is to just replace most instances of 'char' with 'signed char' in numpy_test.pyx: https://github.com/bfroehle/cython/compare/char-buffers
In some sense NumPy does have a notion of 'char', namely a 1 character string.
The only worry here, of course, is whether this change will painfully break existing code…
-Brad
Actually, this fix breaks numpy_test, and I'm not sure how to fix it given that numpy has no notion of a "possibly signed char" (unless we disallow using char/char* with numpy altogether, which is much more extreme, or don't enforce that signs match for char, which is likely more invasive). Ugh.
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Robert Bradshaw, 10.08.2012 23:00:
We have plenty of non-transitive coercions. E.g. char* <-> object <-> float.
Python <-> C coercions are not quite the same league as low-level C type comparisons or coercions, though.
While it's technically more correct to use signed or unsigned char, char* is pretty ubiquitous in the C world.
Absolutely - be it due to correct usage, "don't care" or bad design. Requiring explicit signedness can easily drive this into a const-like nightmare with "but I need it" casts all over the place.
There's also the question of the python bytes object and array.array("c") which would be good to support. And this would disallow any nested chars as parts of structs (e.g. from external libraries--you could fake a signness in your declaration but that seems ugly).
And faking declarations could easily lead to even less portable code.
All in all, particularly in light of interfacing with external code, I think disallowing char is overly strict.
+1 Stefan
On 10 August 2012 19:54, Robert Bradshaw <robertwb@gmail.com> wrote:
On Fri, Aug 10, 2012 at 9:25 AM, Robert Bradshaw <robertwb@gmail.com> wrote:
On Thu, Aug 9, 2012 at 10:04 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 09.08.2012 18:51:
On 9 August 2012 16:36, Stefan Behnel wrote:
Stefan Behnel, 09.08.2012 14:31:
mark florisson, 07.08.2012 11:09: > I thought the 32 bit issue was resolved? You pushed a fix and I fixed > some tests, so it passed for me. I can run it again to check...
I don't know. Yaroslav replied to your mail saying that your fixes didn't change anything for the Debian builds. Let's see what he has to say about the second beta.
He ran them through the build servers. Here are the latest results:
https://buildd.debian.org/status/package.php?p=cython&suite=experimental
(hint: click on "all" in the Logs column, then click on the build result to see the log output)
The i386 tests show 4 errors related to fused types in the NumPy tests:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
From a quick look, they might really be problems in the tests rather than the code we generate.
It also looks like my signed char fix was not sufficient, as you can see in the memslice tests here, e.g. in the "memslice.__test__.test_memslice_struct_with_arrays" test:
https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
It now prints
""" ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char' in 'ArrayStruct.chars' """
Well, "char" and "unsigned char" are supposed to be the same on this platform, but I guess it can't know that. I wonder if "char" and "signed char" are considered identical on other platforms ...
To fix the tests, it might be enough to use an explicit "unsigned char" instead of plain "char" for the types. Not sure if that's a good idea or if there actually is a bug hidden below this test failure. At least, it smells like an inconvenience for users to get that error.
I think these tests are fine (unlike the fused test I fixed), it's a bug somewhere. I'm not sure where though, because the code that creates the format string uses the type info struct (with the C compile-time unsigned check), and so does the buffer format parser. The code creating the type strings are in the MemoryView.pyx and Buffer.c utility files at the bottom. It does
if (type->is_unsigned) *buf = toupper(*buf);
So whatever I feed it on my system (char, signed char or unsigned char), it generates the right format string for me.
I think this needs some more investigation directly on the failing systems. It'll be much easier to figure this out with a debugger.
Strangely enough, I wasn't even able to reproduce this passing "-funsigned-char" so yes, I think it may require access to one of these systems. (Perhaps I'd need to compile Python and/or NumPy with this option as well...)
Perhaps we can just ignore these test failures, I don't think many people are using memoryviews with characters. Changing them to signed or unsigned char may fix the problem for the tests, though.
If we can't find a way to fix this in time, I'm leaning towards that, too. However, working around a bug in a test is a sure way to forget about the bug and consider everything working. Could you copy the failing tests into a separate test case, disable that in bugs.txt and then change the types in the original test modules to whatever you think would make the tests work?
I'll take a stab at this.
OK, the problem boiled down to using 'b' or 'B' for the format string for char rather than 'c'. See, e.g. http://docs.python.org/library/array.html .
That confounds me, I think I need some hand-holding :). So we have a type info struct, from which we create a format string containing 'b' or 'B', depending on signed-ness (at C compile time). We then at buffer format string parsing time use this same type info struct, and we check the size (match) and we check the group of the character in the format string ('b' -> 'I' and 'B' -> 'U') against the expected group from the type info struct (which also depends on the signed-ness). At which point does this go wrong?
I've pushed a fix at https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92a... , but it'd be good to have another set of eyes with someone more familiar with this code to be sure I didn't miss anything. It also involved a lot of changes to the tests https://github.com/robertwb/cython/commit/6bcc8fd8419e6e40793447880230297366... so is not exactly backwards compatible, though perhaps more correct.
I think that's ok, people don't typically write these format strings manually. Thanks for the fixes!
It'd be good to see if this actually fixes the issues.
- Robert _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
participants (6)
-
Bradley M. Froehle -
Dag Sverre Seljebotn -
mark florisson -
Robert Bradshaw -
Stefan Behnel -
Yaroslav Halchenko