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

Memoryviews require more strict contiguous checks then necessary #66635

Closed
seberg mannequin opened this issue Sep 19, 2014 · 23 comments
Closed

Memoryviews require more strict contiguous checks then necessary #66635

seberg mannequin opened this issue Sep 19, 2014 · 23 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@seberg
Copy link
Mannequin

seberg mannequin commented Sep 19, 2014

BPO 22445
Nosy @abalkin, @pitrou, @skrah, @seberg
Files
  • relaxed-strides-checking.patch: Patch relaxing the buffer contiguity checks (tests missing; untested)
  • relaxed-strides-checking.patch: Added missing shape==NULL paths.
  • relaxed-strides-checking.patch: Further fix some empty buffer cases.
  • contiguous.py: Python code for changed contiguity checking (not simpler then C-code, but maybe nice for testing)
  • contiguous.py: Smaller fix, some restructuring.
  • issue22445.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-02-01.14:28:35.921>
    created_at = <Date 2014-09-19.19:00:49.974>
    labels = ['type-feature', 'library']
    title = 'Memoryviews require more strict contiguous checks then necessary'
    updated_at = <Date 2015-02-01.14:28:35.920>
    user = 'https://github.com/seberg'

    bugs.python.org fields:

    activity = <Date 2015-02-01.14:28:35.920>
    actor = 'skrah'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-02-01.14:28:35.921>
    closer = 'skrah'
    components = ['Library (Lib)']
    creation = <Date 2014-09-19.19:00:49.974>
    creator = 'seberg'
    dependencies = []
    files = ['36663', '36676', '36677', '36678', '36680', '36738']
    hgrepos = []
    issue_num = 22445
    keywords = ['patch']
    message_count = 23.0
    messages = ['227113', '227114', '227115', '227116', '227118', '227128', '227129', '227153', '227155', '227168', '227213', '227233', '227262', '227263', '227616', '228228', '228283', '228361', '229443', '229474', '229485', '229728', '235171']
    nosy_count = 5.0
    nosy_names = ['belopolsky', 'pitrou', 'skrah', 'python-dev', 'seberg']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22445'
    versions = ['Python 3.5']

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 19, 2014

    In NumPy we decided some time ago that if you have a multi dimensional buffer, shaped for example 1x10, then this buffer should be considered both C- and F-contiguous. Currently, some buffers which can be used validly in a contiguous fashion are rejected.

    CPython does not support this currently possibly creating smaller nuisance if/once we change it fully in NumPy, see for example numpy/numpy#5085

    I have attached a patch which should (sorry I did not test this at all yet) relax the checks as much as possible. I think this is right, but we did some subtle breaks in user code (mostly cython code) when we first tried changing it, and while numpy arrays may be more prominently C/F-contiguous, compatibility issues with libraries checking for contiguity explicitly and then requesting a strided buffer are very possible.

    If someone could give me a hint about adding tests, that would be great.
    Also I would like to add a small note to the PEP in any case regarding this subtlety, in the hope that more code will take care about such subtleties.

    @seberg seberg mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 19, 2014
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 19, 2014

    There is another oddity: bpo-12845. Does NumPy have a formal definition of
    array contiguity somewhere?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 19, 2014

    BTW, if you have NumPy installed and run test_buffer in Python3.3+,
    numpy.ndarray has many tests against memoryview and _testbuffer.ndarray
    (the latter is our exegesis of PEP-3118).

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 19, 2014

    bpo-12845 should be closed, seems like a bug in some old version. The definition now is simply that the array is contiguous if you can legally access it in a contiguous fashion. Which means first stride is itemsize, second is itemsize*shape[0] for Fortran, inverted for C-order.

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 19, 2014

    To be clear, the important part here, is that to me all elements can be accessed using that scheme. It is not correct to assume that stride[-1] or stride[0] is actually equal to itemsize.

    In other words, you have to be able to pass the pointer to the start of a c-contiguous array into some C-library that knows nothing about strides without any further thinking. The 0-strides badly violate that.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 19, 2014

    Thanks, bpo-12845 is indeed fixed in NumPy.

    Why does NumPy consider an array with a stride that will almost
    certainly lead to undefined behavior (unless you compile with
    -fwrapv) as valid?

    In CPython we try to eliminate these kinds of issues (though
    they may still be present).

    >>> import numpy as np
    import io
    x = np.arange(10)
    y = np.array([x])
    
    print(y.strides)
    (9223372036854775807, 8)
    >>> 
    >>> 
    >>> y.flags
      C_CONTIGUOUS : True
      F_CONTIGUOUS : True
      OWNDATA : True
      WRITEABLE : True
      ALIGNED : True
      UPDATEIFCOPY : False

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 19, 2014

    Well, the 9223372036854775807 is certainly no good for production code and we would never have it in a release version, it is just there currently to expose if there are more problems. However I don't care what happens on overflow (as long as it is not an error).

    Note that the stride here is on a dimension with shape 1. The only valid index is thus always 0 and 0*9223372036854775807=0, so the stride value does not actually matter when calculating offsets into the array. You could simply set it to 80 to get something that would be considered C-contiguous or to 8 to get something that is considered F-contiguous. But both is the case in a way, so just "cleaning up" the strides does not actually get you all the way.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 20, 2014

    Ok, so it is a debug thing in the current NumPy sources.

    IMO ultimately the getbufferproc needs to return valid strides, even
    if the first value isn't used.

    For that matter, the getbufferproc is free to translate the multi-
    dimensional corner case array to a one-dimensional array that is
    automatically C and F-contiguous.

    Does it matter if you lose some (irrelevant?) information about
    the original array structure?

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 20, 2014

    An extra dimension is certainly not irrelevant! The strides *are* valid
    and numpy currently actually commonly creates such arrays when slicing.
    The question is whether or not we want to ignore them for contiguity
    checks even if they have no effect on the memory layout.

    So there are three options I currently see:

    1. Python also generalizes like I would like numpy to end up in the
      future (the current patch should do that) and just don't care about such
      strides, because the actual memory layout is what matters.
    2. We say it is either too dangerous (which may very well be) or you
      want to preserve Fortran/C-order information even when it does not
      matter to the memory layout.

    This leads to this maybe:
    2a) we just keep it as it is and live with minor inconsistencies (or
    never do the relaxed strides in numpy)
    2b) We let these buffers return False on checking for contiguity but
    *allow* allow fetching a buffer when C-/F-contiguous is explicitly asked
    for when getting the buffer. Which is a weird middle way, but it might
    actually be a pretty sane solution (have to think about it).

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 20, 2014

    I think it would help discussing your options if the patch passes test_buffer
    first. Currently it segfaults because shape can be NULL. Also, code in
    memoryobject.c relies on the fact that ndim==0 means contiguous.

    Then, it would help enormously if you give Python function definitions of
    the revised C and F-contiguity.

    I mean something like verify_structure() from Lib/test/test_buffer.py -- that
    function definition was largely supplied by Pauli Virtanen, but I may have
    added the check for strides-is-multiple-of-itemsize (which 2**63-1 usually
    isn't, so the new debug numpy strides don't pass that test).

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 21, 2014

    I am very sorry. The attached patch fixes this (not sure if quite right, but if anything should be more general then necessary). One test fails, but it looks like exactly the intended change.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 21, 2014

    Thanks! I still have to review the patch in depth, but generally
    I'm +1 now for relaxing the contiguity check.

    Curiously enough the existing code already considered e.g. shape=[1], strides=[-5] as contiguous.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 22, 2014

    Since the functions in abstract.c have been committed by Travis Oliphant:

    Could there have been a reason why the {shape=[1], strides=[-5]}
    case was considered but the general case was not?

    Or is it generally accepted among the numpy devs that not considering
    the general case was just an oversight?

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 22, 2014

    Yeah, the code does much the same as the old numpy code (at least most of the same funny little things, though I seem to remember the old numpy code had something yet a bit weirder, would have to check).

    To be honest, I do not know. It isn't implausible that the original numpy code dates back 15 years or more to numeric. I doubt whoever originally wrote it thought much about it, but there may be some good reason, and there is the safety considerations that people use the strides in a way they should not, which may trip us here in any case.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 26, 2014

    Ok, here's my take on the situation:

    1. As far as Python is concerned, shape[0] == 1 was already special-cased, so
      people could not rely on canonical Fortran or C strides anyway.

    2. Accessing an element via strides should be done using PyBuffer_GetPointer(),
      which can of course handle non-canonical strides.

    3. Breakage will only affect NumPy users, since practically no one else is
      using multidimensional arrays.

    Regarding your option 2b): I think it may be confusing, the buffer protocol
    is already so complicated.

    So, I think purity wins here. If you are sure that all future NumPy versions
    will ship with precise contiguity checks, then I'll commit the new patch in 3.5 (earlier versions should not be changed IMO).

    I've moved the checks for 0 in shape[i] to the beginning (len == 0). I hope
    there are no applications that set len incorrectly, but they would be severely
    broken anyway.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Oct 2, 2014

    FWIW, I think it would be good to make this change early in the
    3.5 release cycle, so issues can be found. Sebastian, do you
    have an idea when the change will be decided in numpy?

    Regarding the discussion here ...

    numpy/numpy#5085

    ... about the special stride marker:

    In the case of slicing it would be nice to use the "organic" value
    that would arise normally from computing the slice. That helps in checking other PEP-3118 implementations like Modules/_testbuffer.c
    against numpy.

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Oct 3, 2014

    Numpy 1.9. was only released recently, so 1.10. might be a while. If no
    problems show up during release or until then, we will likely switch it
    by then. But that could end up being a year from now, so I am not sure
    if 3.6 might not fit better. The problems should be mostly mitigated on
    our side. So bug-wise it shouldn't be a big issue I would guess.

    I will try to look at it more soon, but am completly overloaded at least
    for the next few days, and maybe some other numpy devs can chip in. Not
    sure I get your last point, slicing should give the "organic" values
    even for the mangled up thing with relaxed strides on (currently)?!

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Oct 3, 2014

    Okay, the whole thing isn't that urgent either.

    Sorry for the confusion w.r.t slicing: I misremembered what the
    latest numpy version did:

    a)

       >>> x = np.array([[1,2,3,]])
       >>> x.strides
       (9223372036854775807, 8)
    
    
    b)
     
       >>> x = np.array([[1,2,3], [4,5,6]])[:1]
       >>> x.strides
       (24, 8)

    Somehow I thought that case b) would also produce the special marker,
    but it doesn't, so all is well.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 15, 2014

    Is this related to the NPY_RELAXED_STRIDES_CHECKING compilation flag?

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Oct 15, 2014

    @pitrou, yes of course. This would make python do the same thing as numpy does (currently only with that compile flag given).
    About the time schedule, I think I will try to see if some other numpy dev has an opinion. Plus, should look into documenting it for the moment, so that someone who reads up on the buffer protocol should get things right.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 15, 2014

    Like Stefan I think this would be good to go in 3.5. The PyBuffer APIs are relatively new so there shouldn't be a lot of breakage.

    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Oct 20, 2014

    Antoine, sounds good to me, I don't mind this being in python rather sooner then later, for NumPy itself it does not matter I think. I just wanted to warn that there were problems when we first tried to switch in NumPy, which, if I remember correctly, is now maybe 2 years ago (in a dev version), though.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 1, 2015

    New changeset 369300948f3f by Stefan Krah in branch 'default':
    Issue bpo-22445: PyBuffer_IsContiguous() now implements precise contiguity
    https://hg.python.org/cpython/rev/369300948f3f

    @skrah skrah mannequin closed this as completed Feb 1, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant