Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9669

kc611 · 2024-07-24T09:56:20Z

As titled,

This PR removes import guard and adds dunder method support for operator.add with scalar types.

Note: This PR builds on top of #9662

…pied

sklam

First patch of review is for everything but the new_builtins.py.

Suggestions:

I don't think we really need to split pythonapi.py. On both the old and new version, the types are always referring to the C-level types. To minimize the diff, you can swap types.intp to self.py_ssize_t etc. The PythonAPI.__init__ can have all the type definition and the required switching logic for old/new type system.

numba/core/base.py

numba/core/datamodel/new_models.py

numba/core/lowering.py

numba/core/new_pythonapi.py

numba/tests/test_new_type_system.py

sklam

This review is for the __add__, __radd__ on bool, int, float, complex.

I did some digging into the behavior of the binops on the builtin types. They never calls the casting functions e.g. __int__, __float__ in userspace. They are hardcoded to call internal cast that cannot be overridden. For instance, complex ops use: https://github.com/python/cpython/blob/9621a7d0170bf1ec48bcfc35825007cdf75265ea/Objects/complexobject.c#L481-L501

Another example to demonstrate that the __<type>__ method is never called:

class MyOps:
    def __bool__(self):
        print("__bool__")
        return True

    def __int__(self):
        print("__int__")
        return 123

    def __float__(self):
        print("__float__")
        return 12.3

    def __complex__(self):
        print("__complex__")
        return 1   2.3j

class MyInt(MyOps, int):
    pass
class MyFloat(MyOps, float):
    pass
class MyComplex(MyOps, complex):
    pass


assert issubclass(MyInt, int)
print( bool(True)   MyInt(0) )
print( int(10)   MyInt(0) )
print( float(9.1)   MyInt(0) )
print( complex(1.3j)   MyInt(0) )
# Prints:
# 1
# 10
# 9.1
# 1.3j
assert issubclass(MyFloat, float)
print( bool(True)   MyFloat(0) )
print( int(10)   MyFloat(0) )
print( float(9.1)   MyFloat(0) )
print( complex(1.3j)   MyFloat(0) )
# Prints:
# 1.0
# 10.0
# 9.1
# 1.3j
assert issubclass(MyComplex, complex)
print( bool(True)   MyComplex(0) )
print( int(10)   MyComplex(0) )
print( float(9.1)   MyComplex(0) )
print( complex(1.3j)   MyComplex(0) )
# (1 0j)
# (10 0j)
# (9.1 0j)
# 1.3j

There is just no way for user to override these operation base by using the __<type>__() method.

numba/core/typing/new_builtins.py

sklam

This batch of review is from trying to extend __add__ to the Interval type. See https://gist.github.com/sklam/c60a989c927fefd901475a84516f7812 (main code starts at line 171). This is to make sure the protocol-based approach is truely extensible.

numba/core/typing/new_builtins.py

sklam

Reviewed everything up to and including 03c32a0

numba/core/typing/new_builtins.py

sklam · 2024-08-14T22:55:24Z

There are many PEP8 errors introduced in this patch. Can you run the script in #9704 to find and fix these errors?

sklam

The following are violations from flake8:

Run flake8_diff by

git fetch origin pull/9704/head:pr/9704
git checkout pr/9704 maint/flake8_diff.py
python maint/flake8_diff.py

i excluded violations on changes that are clones of old files.

numba/core/new_boxing.py

numba/core/types/misc.py

numba/core/typing/new_builtins.py

sklam · 2024-08-15T17:47:05Z

numba/core/typing/new_builtins.py

- return float_add_float(self, asfloat)
+ if isinstance(other, float):
+ return float_add_float(self, other)
+ elif isinstance(other, (int, bool, np.float64)):


Python float should not know about numpy types. Handling of np.float64 should be done in NumPy binops. Also, the result type would be numpy types.

The primary issue is if we remove this the following behaviour happens:

import numba import numpy as np @numba.njit def foo(v, w): return v.__add__(w) x = 1.1 y = np.float64(5.5) print(foo.py_func(x, y)) # 6.6 print(foo(x, y)) # NotImplemented

sklam · 2024-08-15T17:47:14Z

numba/core/typing/new_builtins.py

+ def impl(self, other):
+ if isinstance(other, complex):
+ return complex_add_complex(self, other)
+ elif isinstance(other, (float, int, bool, np.float64, np.complex128)):


same comment about numpy types

Co-authored-by: Siu Kwan Lam <1929845 [email protected]>

…nd fixed isinstance logic to detect class heirarchy in type system

sklam

Avoid `<type>()` or `<type>()`. Add ways to extract machine repr.

The overloads for __add__ can never call <type>() because they end up calling __<type>__---CPython semantic does not do that. Considering the extensibility of the builtin number types, I think Numba need to either:

restrict these types to retain the datamodel of the base number type. For example, subclasses of int will be a i64 in LLVM. or
implement intrinsics to extract the machine representation for each of the number types. They will be the equivalent of PyFloat_AS_DOUBLE for float.

Test failures

% NUMBA_USE_LEGACY_TYPE_SYSTEM=0 python runtests.py numba/tests/test_new_type_system.py -v
test_add (numba.tests.test_new_type_system.TestDunderMethods.test_add) ... ok
test_dunder_add (numba.tests.test_new_type_system.TestDunderMethods.test_dunder_add) ... FAIL
test_dunder_radd (numba.tests.test_new_type_system.TestDunderMethods.test_dunder_radd) ... FAIL
test_return_types (numba.tests.test_new_type_system.TestTypes.test_return_types) ... ok

======================================================================
FAIL: test_dunder_add (numba.tests.test_new_type_system.TestDunderMethods.test_dunder_add)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/dev/numba/numba/tests/test_new_type_system.py", line 77, in test_dunder_add
    self.assertEqual(res, py_res, (
AssertionError: NotImplemented != (5 5j) : Failed for (1 2j) and (4 3j); gave answer NotImplemented should be (5 5j)

======================================================================
FAIL: test_dunder_radd (numba.tests.test_new_type_system.TestDunderMethods.test_dunder_radd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/dev/numba/numba/tests/test_new_type_system.py", line 93, in test_dunder_radd
    self.assertEqual(res, py_res, (
AssertionError: NotImplemented != (5 5j) : Failed for (1 2j) and (4 3j); gave answer NotImplemented should be (5 5j)

----------------------------------------------------------------------
Ran 4 tests in 60.690s

FAILED (failures=2)

sklam · 2024-08-21T21:11:17Z

numba/core/typing/new_builtins.py

@@ -372,9 360,9 @@ def impl(self):
 def py_bool__add__(self, other):
 def impl(self, other):
 if isinstance(other, bool):
- return bool_add_bool(self, other)
+ return bool_add_bool(self, bool(other))


why does other need to be casted to bool?

There is no bool_add_bool in CPython

>>> True True 2

It is just int_add_int.

In fact, these overloads cannot call int(), bool(), float(), complex() ever. The CPython semantic do not do that. Calling this will allow user to override with __<type>__().

sklam · 2024-08-21T21:51:55Z

numba/core/typing/new_builtins.py

 elif isinstance(other, int):
- return int_add_int(int(self), other)
+ return int_add_int(int(self), int(other))


Cannot call int()

sklam · 2024-08-21T21:52:06Z

numba/core/typing/new_builtins.py

- return float_add_float(self, other)
- elif isinstance(other, (int, bool, np.float64)):
+ if isinstance(other, (bool, int, float)):
+ # Cast is required in case the other is a NumPy float
 return float_add_float(self, float(other))


Cannot call float()

sklam · 2024-08-21T21:54:22Z

numba/core/typing/new_builtins.py

- return complex_add_complex(self, other)
- elif isinstance(other, (float, int, bool, np.float64, np.complex128)):
+ if isinstance(other, (bool, int, float, complex)):
+ # Cast is required in case the other is a NumPy complex
 return complex_add_complex(self, complex(other))


Cannot call complex()

sklam

I put my review into a diff to show what I mean: sklam@d1e4388

Avoid call to context.cast in the operator implementation. Put them into a specific cast intrinsic. We need to control what cast are allowed to match Python semantic.
The diff passes the test but I haven't looked into the changes in NumPy impl.

Added CPython based `__add__` semantics

kc611 · 2024-08-23T08:51:27Z

I added the diff into this PR as a whole.

kc611 marked this pull request as draft July 24, 2024 09:56

kc611 mentioned this pull request Jul 24, 2024

Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9548

Closed

kc611 added the 2 - In Progress label Jul 25, 2024

kc611 force-pushed the dunder_methods branch from edb4da6 to 729948b Compare July 31, 2024 12:22

kc611 added 3 - Ready for Review and removed 2 - In Progress labels Jul 31, 2024

kc611 requested a review from sklam July 31, 2024 15:26

stuartarchibald mentioned this pull request Jul 31, 2024

Type system implementation #1: Added initial implementation for a new type system using redundancies. #9662

Merged

kc611 added 2 - In Progress and removed 3 - Ready for Review labels Aug 12, 2024

kc611 added 9 commits August 12, 2024 17:22

Added dunder methods: add and radd

3d6e48a

copy files to prefix: 'new_'

122c029

copy files to prefix: 'preserved_'

2efbdc7

copy files to prefix: 'old_'

cab6ed4

Merge branches 'new_files', 'old_files' and 'preserved_files' into co…

9de3670

…pied

remove prefix: 'preserved_'

9aeb009

Fixed numba.core.pythonapi

15aa635

Fixed flake8 issues

1ec6c7c

Added test skips for new tests in old type system

01ede3d

kc611 force-pushed the dunder_methods branch from 632af95 to 01ede3d Compare August 12, 2024 12:13

kc611 added 3 - Ready for Review and removed 2 - In Progress labels Aug 12, 2024

kc611 marked this pull request as ready for review August 12, 2024 12:13

kc611 added 2 commits August 13, 2024 00:02

Changed intrinsic add functions to overloads

631c1b0

Corrected config.py file

dd63039

sklam reviewed Aug 12, 2024

View reviewed changes

Addressed review comments

03c32a0

sklam reviewed Aug 14, 2024

View reviewed changes

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

sklam reviewed Aug 14, 2024

View reviewed changes

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

numba/core/typing/new_builtins.py Show resolved Hide resolved

numba/core/typing/new_builtins.py Outdated Show resolved Hide resolved

Addressed review comments

2a384d7

sklam reviewed Aug 15, 2024

View reviewed changes

kc611 and others added 4 commits August 16, 2024 21:24

Apply suggestions from code review

43d3f10

Co-authored-by: Siu Kwan Lam <1929845 [email protected]>

Addressed flake8 issues in diff

a7465fc

Fixed flake8 diff issues

8f83ed2

Made NumPy float64 a subclass of Python float (same for complex128) a…

df89560

…nd fixed isinstance logic to detect class heirarchy in type system

sklam reviewed Aug 21, 2024

View reviewed changes

kc611 and others added 3 commits August 22, 2024 13:11

Added as_*_add functions

f21f0fc

Separated numpy dunder add as it's own intrinsic

be8930d

Review be8930d

d1e4388

sklam reviewed Aug 22, 2024

View reviewed changes

Added CPython based __add__ semantics

59bc4d6

Added CPython based `__add__` semantics

Rollback compiler changes

e735460

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9669

Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9669

kc611 commented Jul 24, 2024

sklam left a comment •

edited

Loading

sklam left a comment

sklam left a comment

sklam left a comment

sklam commented Aug 14, 2024

sklam left a comment

sklam Aug 15, 2024

kc611 Aug 16, 2024 •

edited

Loading

sklam Aug 15, 2024

sklam left a comment

sklam Aug 21, 2024

sklam Aug 21, 2024

sklam Aug 21, 2024

sklam Aug 21, 2024

sklam Aug 21, 2024

sklam Aug 21, 2024

sklam left a comment •

edited

Loading

kc611 commented Aug 23, 2024

Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9669

Are you sure you want to change the base?

Type system implementation #2: Added magic/dunder method support for add operator on scalar types. #9669

Conversation

kc611 commented Jul 24, 2024

sklam left a comment • edited Loading

Choose a reason for hiding this comment

sklam left a comment

Choose a reason for hiding this comment

sklam left a comment

Choose a reason for hiding this comment

sklam left a comment

Choose a reason for hiding this comment

sklam commented Aug 14, 2024

sklam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kc611 Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sklam left a comment

Choose a reason for hiding this comment

Avoid <type>() or __<type>__(). Add ways to extract machine repr.

Test failures

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

sklam left a comment • edited Loading

Choose a reason for hiding this comment

kc611 commented Aug 23, 2024

sklam left a comment •

edited

Loading

kc611 Aug 16, 2024 •

edited

Loading

Avoid `<type>()` or `<type>()`. Add ways to extract machine repr.

sklam left a comment •

edited

Loading