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

Constant slices are deduplicated too aggressively #126298

Closed
befeleme opened this issue Nov 1, 2024 · 9 comments
Closed

Constant slices are deduplicated too aggressively #126298

befeleme opened this issue Nov 1, 2024 · 9 comments
Assignees
Labels
3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@befeleme
Copy link
Contributor

befeleme commented Nov 1, 2024

Bug report

Bug description:

I try to build numpy with Python 3.14.0a1 and encounter very strange test failures regarding array slicing. The offending tests source code can be found here and here.
I compiled both numpy 1.26.4 and 2.1.2 for Fedora Linux and installed it into an environment with Python 3.14. This happens for both versions of numpy. It doesn't happen with Python 3.13 and numpy 1.26.4.

In both tests, it's either TypeError expected and not raised (test_slicing_no_floats), or the other way around (test_valid_slicing). Let's take a look at test_slicing_no_floats. assert_raises is numpy's wrapper over unittest, but that's irrelevant for the problem - let's replace it with a minimal viable function and run the offending piece of code:

>>> def assert_raises(e, c):
...    try:
...        r = c()
...    except e:
...        pass
...    else:
...        raise AssertionError(f"function call: {c}, did not raise: {e}, return value: {r}")
...
>>> def test_slicing_no_floats():
...    a = np.array([[5]])
...    assert_raises(TypeError, lambda: a[0.0:])
...    assert_raises(TypeError, lambda: a[0:, 0.0:2])
...    assert_raises(TypeError, lambda: a[0.0::2, :0])
...    assert_raises(TypeError, lambda: a[0.0:1:2,:])
...    assert_raises(TypeError, lambda: a[:, 0.0:])
...    assert_raises(TypeError, lambda: a[:0.0])
...
>>> test_slicing_no_floats()
Traceback (most recent call last):
  File "<python-input-45>", line 1, in <module>
    test_slicing_no_floats()
    ~~~~~~~~~~~~~~~~~~~~~~^^
  File "<python-input-44>", line 8, in test_slicing_no_floats
    assert_raises(TypeError, lambda: a[:0.0])
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<python-input-34>", line 7, in assert_raises
    raise AssertionError(f"function call: {c}, did not raise: {e}, return code: {r}")
AssertionError: function call: <function test_slicing_no_floats.<locals>.<lambda> at 0x7f9a8a987c10>, did not raise: <class 'TypeError'>, return value: []

What if I switch the 1st and last line of the test? A completely different row stops raising TypeError:

>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[:0.0])
...     assert_raises(TypeError, lambda: a[0:, 0.0:2])
...     assert_raises(TypeError, lambda: a[0.0::2, :0])
...     assert_raises(TypeError, lambda: a[0.0:1:2,:])
...     assert_raises(TypeError, lambda: a[:, 0.0:])
...     assert_raises(TypeError, lambda: a[0.0:])
...     
>>> test_slicing_no_floats()
Traceback (most recent call last):
  File "<python-input-49>", line 1, in <module>
    test_slicing_no_floats()
    ~~~~~~~~~~~~~~~~~~~~~~^^
  File "<python-input-48>", line 7, in test_slicing_no_floats
    assert_raises(TypeError, lambda: a[:, 0.0:])
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<python-input-34>", line 7, in assert_raises
    raise AssertionError(f"function call: {c}, did not raise: {e}, return code: {r}")
AssertionError: function call: <function test_slicing_no_floats.<locals>.<lambda> at 0x7f9a8a987740>, did not raise: <class 'TypeError'>, return value: [[5]]

When I arbitrarily remove the 3rd row from the function, suddenly TypeError is correctly raised:

>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[0.0:])
...     assert_raises(TypeError, lambda: a[0:, 0.0:2])
...     assert_raises(TypeError, lambda: a[0.0:1:2,:])
...     assert_raises(TypeError, lambda: a[:, 0.0:])
...     assert_raises(TypeError, lambda: a[:0.0])
...     
>>> test_slicing_no_floats()
>>>

Other modifications are somewhat unpredictable:

>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[:0.0])
...     assert_raises(TypeError, lambda: a[0:, 0.0:2])
...     assert_raises(TypeError, lambda: a[0.0:1:2,:])
...     assert_raises(TypeError, lambda: a[0.0:])
...     
>>> test_slicing_no_floats()
Traceback (most recent call last):
  File "<python-input-51>", line 1, in <module>
    test_slicing_no_floats()
    ~~~~~~~~~~~~~~~~~~~~~~^^
  File "<python-input-50>", line 6, in test_slicing_no_floats
    assert_raises(TypeError, lambda: a[0.0:])
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<python-input-34>", line 7, in assert_raises
    raise AssertionError(f"function call: {c}, did not raise: {e}, return code: {r}")
AssertionError: function call: <function test_slicing_no_floats.<locals>.<lambda> at 0x7f9a8a987950>, did not raise: <class 'TypeError'>, return value: [[5]]
>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[:0.0])
...     assert_raises(TypeError, lambda: a[0.0:1:2,:])
...     assert_raises(TypeError, lambda: a[0.0:])
...     
>>> test_slicing_no_floats()  # no problem
>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[:0.0])
...     assert_raises(TypeError, lambda: a[0.0:1:2,:])
...     
>>> test_slicing_no_floats() # no problem
>>> def test_slicing_no_floats():
...     a = np.array([[5]])
...     assert_raises(TypeError, lambda: a[:0.0])
...     
>>> test_slicing_no_floats() # no problem
>>>

I tried also direct slicing, in all cases TypeError was correctly raised:

>>> a = np.array([[5]])
>>> a[:0.0]
Traceback (most recent call last):
  File "<python-input-18>", line 1, in <module>
    a[:0.0]
    ~^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

And calling lambdas with the same result:

>>> (lambda: a[0.0::2, :0])()
Traceback (most recent call last):
  File "<python-input-59>", line 1, in <module>
    (lambda: a[0.0::2, :0])()
    ~~~~~~~~~~~~~~~~~~~~~~~^^
  File "<python-input-59>", line 1, in <lambda>
    (lambda: a[0.0::2, :0])()
             ~^^^^^^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method
The original test failure from our build system

    def test_slicing_no_floats():
        a = np.array([[5]])
    
        # start as float.
        assert_raises(TypeError, lambda: a[0.0:])
        assert_raises(TypeError, lambda: a[0:, 0.0:2])
        assert_raises(TypeError, lambda: a[0.0::2, :0])
        assert_raises(TypeError, lambda: a[0.0:1:2,:])
        assert_raises(TypeError, lambda: a[:, 0.0:])
        # stop as float.
>       assert_raises(TypeError, lambda: a[:0.0])

a          = array([[5]])
self       = <numpy.core.tests.test_indexing.TestIndexing object at 0x7f7e1a3bfd90>

../../../BUILDROOT/usr/lib64/python3.14/site-packages/numpy/core/tests/test_indexing.py:54: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/lib64/python3.14/unittest/case.py:804: in assertRaises
    return context.handle('assertRaises', args, kwargs)
        args       = (<function TestIndexing.test_slicing_no_floats.<locals>.<lambda> at 0x7f7e081dae50>,)
        context    = None
        expected_exception = <class 'TypeError'>
        kwargs     = {}
        self       = <numpy.testing._private.utils._Dummy testMethod=nop>
/usr/lib64/python3.14/unittest/case.py:237: in handle
    with self:
        args       = []
        callable_obj = <function TestIndexing.test_slicing_no_floats.<locals>.<lambda> at 0x7f7e081dae50>
        kwargs     = {}
        name       = 'assertRaises'
        self       = None
/usr/lib64/python3.14/unittest/case.py:260: in __exit__
    self._raiseFailure("{} not raised by {}".format(exc_name,
        exc_name   = 'TypeError'
        exc_type   = None
        exc_value  = None
        self       = <unittest.case._AssertRaisesContext object at 0x7f7e07ca8dd0>
        tb         = None
        
    # here no TypeError is expected, yet it is raised
    def test_valid_slicing(self):
        # These should raise no errors.
        a = np.array([[[5]]])
    
        a[::]
>       a[0:]
E       TypeError: slice indices must be integers or None or have an __index__ method

CPython versions tested on:

3.14

Operating systems tested on:

Linux

Linked PRs

@befeleme befeleme added the type-bug An unexpected behavior, bug, or error label Nov 1, 2024
@picnixz picnixz added tests Tests in the Lib/test dir and removed tests Tests in the Lib/test dir labels Nov 1, 2024
@picnixz

This comment was marked as outdated.

@picnixz picnixz added 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 1, 2024
@JelleZijlstra
Copy link
Member

What build options are you using (e.g. JIT, free-threading)? Wonder if this could be #126222.

@hroncok
Copy link
Contributor

hroncok commented Nov 1, 2024

JIT is built but not enabled (--enable-experimental-jit=yes-off is used), GIL is enabled (--disable-gil is not used).

@Zheaoli
Copy link
Contributor

Zheaoli commented Nov 1, 2024

What build options are you using (e.g. JIT, free-threading)? Wonder if this could be #126222.

I guess this is a different issue with #126222. Different behavior. I'm trying to reproduce it on my local branch

@brandtbucher
Copy link
Member

No-NumPy reproducer:

class GetItemPrinter:
    def __getitem__(self, key):
        print(key) 

a = GetItemPrinter()
a[0.0:]
a[0:, 0.0:2]
a[0.0::2, :0]
a[0.0:1:2,:]
a[:, 0.0:]
a[:0.0]

Output:

slice(0.0, None, None)
(slice(0.0, None, None), slice(0.0, 2, None))
(slice(0.0, None, 2), slice(None, 0, None))
(slice(0.0, 1, 2), slice(None, None, None))
(slice(None, None, None), slice(0.0, None, None))
slice(None, 0, None)

I believe this is due to GH-125063 (CC @mdboom). In the bytecode compiler, we deduplicate constants. However, it's not enough to deduplicate them by equality; we also need to disambiguate based on type, signed zero, etc. so that 0, 0j, -0.0, 0.0, and False all become different constants. All of this happens recursively for containers like tuples and frozensets, so that (0,) and (False,) are distinct too. The logic is in _PyCode_ConstantKey.

It looks like slices are only being compared by equality, which means that slice(0, None, None) is being deduplicated with slice(0.0, None, None). We should probably do something similar to tuples here.

@brandtbucher brandtbucher changed the title Magically disappearing TypeErrors in numpy on Python 3.14 Constant slices are deduplicated too aggressively Nov 1, 2024
@mdboom mdboom self-assigned this Nov 4, 2024
@mdboom
Copy link
Contributor

mdboom commented Nov 4, 2024

Thanks for finding this. I'll have a look.

@brandtbucher
Copy link
Member

One observation we had while discussing this is that, in general _PyCode_ConstantKey is subtle and error-prone. A possible improvement would be to just have it return PyMarshal_WriteObjectToString(op, Py_MARSHAL_VERSION), since we know these constants can be marshalled and the resulting byte string has the required equality properties.

@mdboom
Copy link
Contributor

mdboom commented Nov 4, 2024

One observation we had while discussing this is that, in general _PyCode_ConstantKey is subtle and error-prone. A possible improvement would be to just have it return PyMarshal_WriteObjectToString(op, Py_MARSHAL_VERSION), since we know these constants can be marshalled and the resulting byte string has the required equality properties.

Yeah, that's a good idea. I think we can merge this PR (assuming it's correct), and then investigate the marshal approach. I'm happy to take that on if no one else wants to.

@mdboom
Copy link
Contributor

mdboom commented Nov 4, 2024

There's also intern_constants, which in a free-threaded build is almost the same as this. If possible, removing both would be nice to do.

mdboom added a commit that referenced this issue Nov 7, 2024
* gh-126298: Don't deduplicated slice constants based on equality

* NULL check for PySlice_New

* Fix refcounting

* Fix refcounting some more

* Fix refcounting

* Make tests more complete

* Fix tests
@mdboom mdboom closed this as completed Nov 7, 2024
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
…ython#126398)

* pythongh-126298: Don't deduplicated slice constants based on equality

* NULL check for PySlice_New

* Fix refcounting

* Fix refcounting some more

* Fix refcounting

* Make tests more complete

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

7 participants