ENH: add check_size option to ctypedef class for external classes #2627

mattip · 2018-09-24T23:04:14Z

Address #2221 by adding a new check_size clause to ctypedef class definitions. This will allow suppressing the warning.

The srctree test actually comprises a few tests, but they are reported as one pass/fail. I am not sure how to break them into separate tests.

I think 'min', True, and False were the proposed values, but maybe I misunderstood.

mattip · 2018-09-24T23:06:25Z

Of course, we can fix the original issue by using this in numpy.__init__.pxd.

mattip · 2018-09-24T23:20:51Z

Test failures from cpp, did I cause that?

compiling (cpp) and running extmember ... 
=== C/C++ compiler output: =========
pointers.cpp: In function ‘PyObject* __pyx_pf_8pointers_compare(PyObject*)’:
pointers.cpp:1135:65: warning: self-comparison always evaluates to true [-Wtautological-compare]

mattip · 2018-09-25T00:04:19Z

Yes, it seems filename.pxd creates a filename_api.h without the proto section, so it misses the declaration of the enum I tried to create.

scoder

Very nicely written PR, sorry for the load of comments. I think we should still get this into 0.29.
The travis failure is just some whitespace left-over, found by the style checker.

scoder · 2018-09-25T04:52:40Z

Cython/Utility/ImportExport.c

+     * check_size tells what to do if tp_basicsize is different from size:
+     * 0 - Error (originates in check_size=True)
+     * 1 - Error if tp_basicsize is smaller, warn if larger (originates in check_size='min')
+     * 2 - Error if tp_basicsize is smaller (originates in check_size=False)


Hmm, this doesn't look like what I'd expect a check_size=False to do … I mean, it's probably the right behaviour, but then False is not the right setting. (Open for debate, I guess …)

I think we always need to error if tp_basicsize is smaller than the C struct declared as the object, see the _check_size1.pyx test where I define a C struct that is bigger. Accessing f2 would touch invalid memory.

So I claim even if using False we need to error on the side of caution.Open to other suggestions for the name.

If I understand the options correctly, we always error fo smaller structs, but either ignore, warn, or error for larger structs. This doesn't map obviously to the True/False/'min' options provided here--maybe literally call then None, 'warn,' and 'error'? (As an aside, maybe it'd be better to use an enum than 0, 1, 2 as well).

(I actually wonder if instead we'd be better off doing a stricter check using offsetof for declared fields, even for non-extern classes, but that may have issues...)

What about "extend", "warn" (default) and "error" ? None doesn't tell me much either.

Agreed that offset checks would be nice, but that's a separate feature (and not for 0.29).

maybe it'd be better to use an enum than 0, 1, 2

I tried that, but an enum just before the __Pyx_ImportType declaration in the TypeImport.proto section was not exported to the generated code. Is there an example of such an enum?

scoder · 2018-09-25T04:57:11Z

docs/src/userguide/extension_types.rst

@@ -736,6 +736,14 @@ built-in complex object.::
            ...
        } PyComplexObject;

+       At runtime, a check will be performed when importing the cython
+       c-extension module that ``__builtin__.complex``'s ``tp_basicsize``
+       matches ``sizeof(`PyComplexObject)``. This check can fail if in the cython


No "in", just "if the Cython …".

scoder · 2018-09-25T04:58:13Z

docs/src/userguide/extension_types.rst

@@ -736,6 +736,14 @@ built-in complex object.::
            ...
        } PyComplexObject;

+       At runtime, a check will be performed when importing the cython


Please spell the name "Cython" with a capital "C". (You are not referring to the command line utility here.)

scoder · 2018-09-25T04:59:21Z

docs/src/userguide/extension_types.rst

+       c-extension module that ``__builtin__.complex``'s ``tp_basicsize``
+       matches ``sizeof(`PyComplexObject)``. This check can fail if in the cython
+       c-extension module was compiled with one version of the
+       ``complexobject.h`` header but imported into a python with a changed


Same thing, I think you are referring to a "Python" runtime here, not the "python" executable.

scoder · 2018-09-25T05:03:06Z

Cython/Compiler/Parsing.py

        in_pxd = ctx.level == 'module_pxd',
        doc = doc,
        body = body)

 def p_c_class_options(s):
    objstruct_name = None
    typeobj_name = None
+    check_size = b'min'


I'd like to have this a normal text string rather than a bytes string. Comparison to bytes might fail then parsed from Py3 code with unicode literals.
One way to do that might be to use the identifier min instead of the string literal "min". Or check the literal node type and decode at need.

it is messy since we want min, True, or False. Changing from p_atom(s) to p_ident(s) makes True and False into strings not booleans, is that OK? In any case, that enables removing the b (as long as we use a bare min not 'min'. Changed documentation (which was missing quotes anyway) and test too.

scoder · 2018-09-25T05:08:14Z

docs/src/userguide/extension_types.rst

+- ``type_object_name`` is the name to assume for the type's statically
+  declared type object.
+- ``cs_option`` is ``'min'`` (the default), ``True``, or ``False`` and is only
+  used for external extension types. If ``True``, ``sizeof(object_struct)`` must


"only used for external extension types" - does the patch check that it's not used in other contexts? (And, are there actually other contexts?)

I was relating to the if not type.is_external or type.is_subclassed check done in ModuleNode.py, which I admit I don't really understand. See last comment below. A suggestion for rephrasing that better catches the logic is welcome

scoder · 2018-09-25T05:09:52Z

runtests.py

-                    command, self._try_decode(out), self._try_decode(err)))
+                for c, o, e in zip(cmd, out, err):
+                    sys.stderr.write("%s\n%s\n%s\n\n" % (
+                        c, self._try_decode(o), self._try_decode(e)))


This was already part of the other PR, right?

yes, sorry. Removing.

scoder · 2018-09-25T05:15:38Z

Cython/Compiler/ModuleNode.py

-            type.typeptr_cname,
-            error_code))
+        # check_size
+        if not type.is_external or type.is_subclassed:


Would be better to raise a compiler error here if check_size was set. Or at least warn that the value was ignored.

I am not sure what these values mean. It is from the original code and the source of the documentation problem you pointed out.

Check added, test needed. Suggestion for ctypedef stanza that will trigger the failure welcome.

"is_external" = defined in a "cdef extern" block. False if a type is cimported from another module via a .pxd file.
"is_subclassed" = we found a subtype of the type, i.e. it is definitely not a leaf class of the hierarchy.

Or do we want to allow this for non-extern classes? (We need to take care that the vtable doesn't change either...)

If the type is cimported, how would we set check_size? Is there an example in the tests that redefines a cimported type?

mattip · 2018-09-25T06:41:09Z

Maybe worth adding some additional tests for failures and corner cases? Can we somehow write them as non-srctree tests?

scoder · 2018-09-25T16:55:57Z

Can we somehow write them as non-srctree tests?

Don't think so. You need at least two files in order to cimport from the other.
You migth be able to cover multiple cases with the same srctree file, though. These tests are quite expensive to run, so it's not uncommon to cover an entire (little) feature with one test setup.

scoder · 2018-09-25T16:44:23Z

Cython/Compiler/ModuleNode.py

-            type.typeptr_cname,
-            error_code))
+        # check_size
+        if not type.is_external or type.is_subclassed:


"is_external" = defined in a "cdef extern" block. False if a type is cimported from another module via a .pxd file.
"is_subclassed" = we found a subtype of the type, i.e. it is definitely not a leaf class of the hierarchy.

Cython/Compiler/Parsing.py

mattip · 2018-09-29T08:29:11Z

Thanks for merging. Now on to using this in the numpy pxd ...

scoder · 2018-09-29T08:51:36Z

Could you quickly write up a PR for that? I'd like to get the second RC out today.

scoder · 2018-09-29T11:19:03Z

Actually, I'll just enable check_size=False for the ndarray and dtype types, that should be it.

mattip · 2018-09-29T16:13:15Z

Thanks @scoder

…ous: "warn" warns, "error" fails, and "extend" silently allows extending. Closes #2627.

scoder · 2018-09-29T17:09:44Z

I changed the available options to "extend", "warn" and "error". The boolean values really were everything but obvious.

robertwb · 2018-09-29T20:06:26Z

Perhaps the options should be "ignore", "error", and "warn"? Not sure why putting an enum right before the __Pyx_ImportType declaration didn't work, that's what I would have done. As for non-extern classes, it would help if one compiled against an an older version of a pxd. One would still have to ensure the vtables are compatible, and of course it would have to be final. Just a half-baked thought there, something we could always flesh out and enable later if there was a need. Speaking of which, are we marking these no-size-check classes as final?

…

On Sat, Sep 29, 2018 at 7:21 PM Matti Picus ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Cython/Utility/ImportExport.c <#2627 (comment)>: > { + /* + * check_size tells what to do if tp_basicsize is different from size: + * 0 - Error (originates in check_size=True) + * 1 - Error if tp_basicsize is smaller, warn if larger (originates in check_size='min') + * 2 - Error if tp_basicsize is smaller (originates in check_size=False) maybe it'd be better to use an enum than 0, 1, 2 I tried that, but an enum just before the __Pyx_ImportType declaration in the TypeImport.proto section was not exported to the generated code. Is there an example of such an enum? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2627 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdqgRynXi1GQm9sgpT_f_ZJnQNP60iZks5uf6wTgaJpZM4W3mSh> .

scoder · 2018-09-30T05:13:08Z

Perhaps the options should be "ignore", "error", and "warn"?

Works for me, feel free to change it. I already reduced the number of places that need to be adapted.

mattip added 2 commits September 25, 2018 01:29

ENH: add check_size option to ctypedef class for external classes

14387ec

DOC: document the new check_size clause

b27baf0

MAINT: cannot use local enum in __Pyx functions

37fb8a2

scoder reviewed Sep 25, 2018

View reviewed changes

scoder added this to the 0.29 milestone Sep 25, 2018

scoder added enhancement Cython Language Feature labels Sep 25, 2018

MAINT: fixes from review

920aee8

scoder reviewed Sep 25, 2018

View reviewed changes

MAINT: fixes from review

9e7fdef

mattip mentioned this pull request Sep 26, 2018

ENH: Generalized ufunc signature expansion for frozen and flexible dimensions numpy/numpy#11175

Merged

scoder merged commit a699043 into cython:master Sep 29, 2018

scoder added a commit that referenced this pull request Sep 29, 2018

Minor cleanups of 'check_size' implementation (#2627).

df1b244

This was referenced Sep 29, 2018

add compile option to hide binary size warnings #2221

Closed

RuntimeWarning: numpy.dtype size changed, may indicate binary incompatibility. numpy/numpy#11788

Closed

scoder added a commit that referenced this pull request Sep 29, 2018

Rename the options of the "check_size" feature to make them more obvi…

c9eda12

…ous: "warn" warns, "error" fails, and "extend" silently allows extending. Closes #2627.

mattip deleted the check_size2 branch October 29, 2018 09:12

henryiii mentioned this pull request May 18, 2019

Distribute iminuit as a wheel on PyPI scikit-hep/iminuit#334

Closed

mattip mentioned this pull request Aug 3, 2020

ENH,API: Store exported buffer info on the array numpy/numpy#16938

Merged

ENH: add check_size option to ctypedef class for external classes #2627

ENH: add check_size option to ctypedef class for external classes #2627

mattip commented Sep 24, 2018

mattip commented Sep 24, 2018

mattip commented Sep 24, 2018

mattip commented Sep 25, 2018

scoder left a comment •

edited

scoder Sep 25, 2018

mattip Sep 25, 2018

robertwb Sep 29, 2018

scoder Sep 29, 2018 •

edited

mattip Sep 29, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

mattip Sep 25, 2018

mattip Sep 25, 2018

scoder Sep 25, 2018

robertwb Sep 29, 2018

mattip Sep 29, 2018

mattip commented Sep 25, 2018

scoder commented Sep 25, 2018

scoder Sep 25, 2018

mattip commented Sep 29, 2018

scoder commented Sep 29, 2018

scoder commented Sep 29, 2018

mattip commented Sep 29, 2018

scoder commented Sep 29, 2018

robertwb commented Sep 29, 2018 via email

scoder commented Sep 30, 2018 via email

ENH: add check_size option to ctypedef class for external classes #2627

ENH: add check_size option to ctypedef class for external classes #2627

Conversation

mattip commented Sep 24, 2018

mattip commented Sep 24, 2018

mattip commented Sep 24, 2018

mattip commented Sep 25, 2018

scoder left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoder Sep 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented Sep 25, 2018

scoder commented Sep 25, 2018

Choose a reason for hiding this comment

mattip commented Sep 29, 2018

scoder commented Sep 29, 2018

scoder commented Sep 29, 2018

mattip commented Sep 29, 2018

scoder commented Sep 29, 2018

robertwb commented Sep 29, 2018 via email

scoder commented Sep 30, 2018 via email

scoder left a comment •

edited

scoder Sep 29, 2018 •

edited