Skip to content

Support for large numbers of args/kws with Python 3.10 #7866

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.

Sign up for GitHub

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

Merged
sklam merged 24 commits into from
May 6, 2022
Merged

Support for large numbers of args/kws with Python 3.10 #7866

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7ad9642
added basic support for args <= 30 and kws <= 15
Feb 22, 2022
ebea710
Support for kws > 15
Feb 22, 2022
3107840
working with args > 30, needs refactoring + error checking
Feb 23, 2022
fd6669b
Refactored the code into helpful functions, still need to write tests
Feb 25, 2022
f3ad535
added tests
Feb 25, 2022
29953e7
added Ehsan's comments
Feb 28, 2022
28a22ee
fixed small bug in skip_unless_py10
Mar 1, 2022
f726b3a
updated the comments and tests
Mar 2, 2022
974da74
added an objmode test
Apr 18, 2022
dbb4dec
fixed the definition updates
Apr 18, 2022
7231d71
Add Siu's changes
Apr 18, 2022
2a01061
prepared to add extra tests
Apr 18, 2022
1d38efa
updated the error message and added additional tests
Apr 18, 2022
f766532
Added tests to check for inlined control flow, doesn't handle only args
Apr 18, 2022
7e8d424
added error handling for the inline case with all args
Apr 18, 2022
cd04a5e
simplified the error message sources
Apr 18, 2022
224c514
fix pytest assertion
Apr 18, 2022
9d234a2
updated the error message
Apr 28, 2022
af48701
Update numba/core/interpreter.py
njriasan Apr 28, 2022
9ed27ea
made Stuart's test changes
May 4, 2022
530284e
apply stuart's interpreter changes
May 4, 2022
9bb2ee5
Merge branch 'nick/16_kwargs_py310_peephole' of https://github.com/nj…
May 4, 2022
ba83ffb
applied remaining comment changes
May 4, 2022
265a426
Made Stuart's final changes
May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
added error handling for the inline case with all args
  • Loading branch information
Nicholas J Riasanovsky
Nicholas J Riasanovsky committed Apr 18, 2022
commit 7e8d424921331d193ab6fe0f752af6a72075480a
38 changes: 38 additions & 0 deletions numba/core/interpreter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,44 @@ def peep_hole_call_function_ex_to_call_function_kw(func_ir):
stmt = ir.Assign(new_call, stmt.target, stmt.loc)
# Update the definition
func_ir._definitions[stmt.target.name].append(new_call)
elif (
isinstance(stmt, ir.Assign)
and isinstance(stmt.value, ir.Expr)
and stmt.value.op == "call"
and stmt.value.vararg is not None
):
# If there is a call with vararg we need to check
# if the list -> tuple conversion failed and if so
# throw an error.
call = stmt.value
args = call.args
vararg = call.vararg
if args:
# If we have vararg then args is expected to
# be an empty list.
raise UnsupportedError(errmsg)
vararg_loc = i - 1
args_def = None
not_found = True
while vararg_loc >= 0 and not_found:
args_def = blk.body[vararg_loc]
if (
isinstance(args_def, ir.Assign)
and args_def.target.name == vararg.name
):
not_found = False
else:
vararg_loc -= 1

@sklam sklam Apr 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Leaving thought trail in case we need it in the future. This probably doesn't present to be a problem)

With bytecode from:

args = (*(0, 1, 2),)   # the starunpack triggers LIST_TO_TUPLE
y = my_func2(*args)
 16           0 BUILD_LIST               0
              2 LOAD_CONST               1 ((0, 1, 2))
              4 LIST_EXTEND              1
              6 LIST_TO_TUPLE
              8 STORE_FAST              31 (args)

 17          10 LOAD_GLOBAL              0 (my_func2)
             12 LOAD_FAST               31 (args)
             14 CALL_FUNCTION_EX         0
             16 STORE_FAST              32 (y)

the code here scan back from the call to my_func2 to find the definition of args, which is suppose to be a LIST_TO_TUPLE opcode, but is replaced by peep_hole_list_to_tuple; otherwise, the later check will reject this call.

@sklam sklam Apr 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also tried to trick this part of the code with the walrus operator but Numba is already miscompiling this on mainline:

my_func2(args:=(*(0, 1, 2),), *args)

if not_found:
# If we couldn't find where the args are created
# then we can't handle this format.
raise UnsupportedError(errmsg)
else:
# If this value is still a list to tuple raise the
# exception.
expr = blk.body[vararg_loc].value
if isinstance(expr, ir.Expr) and expr.op == "list_to_tuple":
raise UnsupportedError(errmsg)

new_body.append(stmt)
# Update the block body if we updated the IR

@stuartarchibald stuartarchibald May 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replacing, not updating?

@njriasan njriasan May 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Expand Down
117 changes: 116 additions & 1 deletion numba/tests/test_interpreter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ def inline_func(flag):
# self.assertEqual(a, b)

@skip_unless_py10
def test_large_kws_uninlined_controlflow(self):
def test_large_args_uninlined_controlflow(self):

@stuartarchibald stuartarchibald May 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_large_args_uninlined_controlflow(self):
def test_large_args_noninlined_controlflow(self):

(suggest same change throughout).

@njriasan njriasan May 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"""
Tests generating large args when one of the inputs
has the change suggested in the error message
Expand Down Expand Up @@ -450,6 +450,121 @@ def inline_func(flag):
b = cfunc(False)
self.assertEqual(a, b)

@skip_unless_py10
def test_all_args_inline_controlflow(self):
"""
Tests generating only large args when one of the inputs
has inlined controlflow. This requires a special check
inside peep_hole_call_function_ex_to_call_function_kw
because it usually only handles varkwargs.
"""
def inline_func(flag):
return sum_jit_func(
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1 if flag else 2,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
)

with pytest.raises(
UnsupportedError,
match="You can resolve this issue by moving the control flow out"
):
njit()(inline_func)(False)
# Uncomment sections when inlined control flow is
# actually supported.
# py_func = inline_func
# cfunc = njit()(inline_func)
# a = py_func(False)
# b = cfunc(False)
# self.assertEqual(a, b)

@stuartarchibald stuartarchibald May 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove? Same reasoning as above.

@njriasan njriasan May 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


@skip_unless_py10
def test_all_args_uninlined_controlflow(self):
"""
Tests generating large args when one of the inputs
has the change suggested in the error message
for inlined control flow.
"""
def inline_func(flag):
a_val = 1 if flag else 2
return sum_jit_func(
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
a_val,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
1,
)

py_func = inline_func
cfunc = njit()(inline_func)
a = py_func(False)
b = cfunc(False)
self.assertEqual(a, b)

@skip_unless_py10
def test_large_kws_inline_controlflow(self):
"""
Expand Down