Add support for positional args with args/kwargs #611

jagerman · 2017-01-22T05:34:06Z

This commit rewrites the function dispatcher code to support mixing regular arguments with py::args/py::kwargs arguments. It also simplifies the argument loader noticeably as it no longer has to worry about args/kwargs: all of that is now sorted out in the dispatcher, which now simply appends a tuple/dict if the function takes py::args/py::kwargs, then passes all the arguments in a single tuple.

Some (intentional) restrictions:

you may not bind a function that has args/kwargs somewhere other than the end. This somewhat matches Python, but mainly keeps the dispatch code a little cleaner by being able to not worry about where to inject the py::args/py::kwargs in the argument list. Fully matching what Python allows (i.e. named, but not positional, arguments after *args) seems not worth bothering with.
If you specify an argument both positionally and via a keyword argument, you get a TypeError (matching what happens in Python).

jagerman · 2017-01-22T14:14:54Z

Pushed a second commit to avoid needing to create a tuple in favour of a lighter std::vector<handle>. Since this argument list is never going to Python, there's little point in using a tuple (the previous version used a tuple mainly because so that it could simply pass that tuple on when the size was just right).

dean0x7d

Nice. Having the dispatcher return all the args in a single container makes a lot of sense. +1 for std::vector.

dean0x7d · 2017-01-22T18:51:31Z

include/pybind11/cast.h

+    : assert_args_kwargs_must_be_last<false, true, sizeof...(T) == 0> {};
+template <typename... T> struct args_kwargs_must_be_last<args, kwargs, T...>
+    : assert_args_kwargs_must_be_last<true, true, sizeof...(T) == 0> {};
+template <> struct args_kwargs_must_be_last<> : assert_args_kwargs_must_be_last<false, false, true> {};


Instead of the custom metafunction, couldn't this reuse constexpr_last and constexpr_first which are already used for a similar purpose in the unpacking_collector version of collect_arguments()?

Hmm, I tried to figure out a nicer way to do that, but couldn't think of one. But actually, let me look again, I have an idea...

Aha, I didn't realize we had constexpr_first. I have to move it anyway (it's defined after the argument loader), so I'm going to move it to common.h (to put it with constexpr_sum).

dean0x7d · 2017-01-22T19:09:56Z

include/pybind11/pybind11.h

-        static_assert(detail::expected_num_args<Extra...>(sizeof...(Args)),
+        constexpr bool have_args   = detail::any_of<std::is_same<args,   detail::intrinsic_t<Args>>...>::value,
+                       have_kwargs = detail::any_of<std::is_same<kwargs, detail::intrinsic_t<Args>>...>::value;
+        static_assert(detail::expected_num_args<Extra...>(sizeof...(Args), have_args, have_kwargs),


There's no need to recompute have_args and has_kwargs -- the info is already available as cast_in::has_args and cast_in::has_kwargs. The static_assert just needs to be placed after using cast_in.

Oh, of course it is! Good catch.

dean0x7d · 2017-01-22T19:57:05Z

include/pybind11/pybind11.h

                        }
                    }
+                    pass_args[args_copied++] = extra_args;


Consider .reserve() + .push_back() as more idiomatic C++, in place of pass_args[args_copied++]. Unless this complicates matters elsewhere, then dissregard. (But generally with .reserve() it should be args_copied == pass_args.size()).

I had it that way originally, but then I figured this way might be slightly more optimal as I know the number of elements I need exactly (and so avoid the std::vector needing to update its internal size state variable n times). But in retrospect, that's probably microoptimization (and I'm not accounting for the fact that as as I need to invoke n empty handles and then invoke n handle copy assignments.) So yeah, I'll change it back.

dean0x7d · 2017-01-22T20:02:14Z

tests/test_kwargs_and_defaults.py

+    with pytest.raises(TypeError):
+        assert mpakd(1, i=1)
+    with pytest.raises(TypeError):
+        assert mpakd(1, 2, j=1)


It would be good to also verify that the error messages are correctly formed.

jagerman · 2017-01-23T00:54:02Z

@dean0x7d - is there any simple way I can mute that line length error (see the build failure)? This doesn't really seem like a case I can easily shorten.

dean0x7d · 2017-01-23T00:58:22Z

Yes, with noqa like in this file.

This commit rewrites the function dispatcher code to support mixing regular arguments with py::args/py::kwargs arguments. It also simplifies the argument loader noticeably as it no longer has to worry about args/kwargs: all of that is now sorted out in the dispatcher, which now simply appends a tuple/dict if the function takes py::args/py::kwargs, then passes all the arguments in a vector. When the argument loader hit a py::args or py::kwargs, it doesn't do anything special: it just calls the appropriate type_caster just like it does for any other argument (thus removing the previous special cases for args/kwargs). Switching to passing arguments in a single std::vector instead of a pair of tuples also makes things simpler, both in the dispatch and the argument_loader: since this argument list is strictly pybind-internal (i.e. it never goes to Python) we have no particular reason to use a Python tuple here. Some (intentional) restrictions: - you may not bind a function that has args/kwargs somewhere other than the end (this somewhat matches Python, and keeps the dispatch code a little cleaner by being able to not worry about where to inject the args/kwargs in the argument list). - If you specify an argument both positionally and via a keyword argument, you get a TypeError alerting you to this (as you do in Python).

Passing a negative value wasn't valid anyway, and moreover this avoids a little bit of extra code to avoid signed/unsigned argument warnings.

This keeps it with constexpr_sum and the other metafunctions.

jagerman · 2017-01-23T01:05:37Z

Perfect. I squashed everything, incorporating all your suggestions.

This cleans up the previous commit slightly by further reducing the function call arguments to a single struct (containing the function_record, arguments vector, and parent). Although this doesn't currently change anything, it does allow for future functionality to have a place for precalls to store temporary objects that need to be destroyed after a function call (whether or not the call succeeds). As a concrete example, with this change pybind#625 could be easily implemented (I think) by adding a std::unique_ptr<gil_scoped_release> member to the `function_call` struct with a precall that actually constructs it. Without this, the precall can't do that: the postcall won't be invoked if the call throws an exception. This doesn't seems to affect the .so size noticeably (either way).

wjakob · 2017-01-31T16:27:34Z

Thank you, this is amazing -- the new dispatch code looks very simple and clean. Merged just now outside of github to fix a conflict manually.

jagerman mentioned this pull request Jan 22, 2017

Eigen/numpy referencing #610

Merged

jagerman force-pushed the args-kwargs-posargs-mixing branch 2 times, most recently from 9a4c6a4 to 80b04cb Compare January 22, 2017 06:01

dean0x7d reviewed Jan 22, 2017

View reviewed changes

jagerman force-pushed the args-kwargs-posargs-mixing branch from ed90543 to cea4f42 Compare January 23, 2017 00:57

jagerman added 4 commits January 22, 2017 20:04

Changed keep_alive template arguments from int to size_t

ee44504

Passing a negative value wasn't valid anyway, and moreover this avoids a little bit of extra code to avoid signed/unsigned argument warnings.

Move constexpr_first/last to common.h

d28925a

This keeps it with constexpr_sum and the other metafunctions.

Use constexpr_first for args/kwargs positional checks

9755491

jagerman force-pushed the args-kwargs-posargs-mixing branch from e5c168c to 9755491 Compare January 23, 2017 01:04

jagerman mentioned this pull request Jan 30, 2017

Provide overload similar to return_value_policy to unlock the GIL #625

Closed

jagerman force-pushed the args-kwargs-posargs-mixing branch from 37cf37b to a3ac042 Compare January 30, 2017 20:59

jagerman force-pushed the args-kwargs-posargs-mixing branch from a3ac042 to 59ecf31 Compare January 30, 2017 22:35

wjakob closed this Jan 31, 2017

jagerman mentioned this pull request Feb 23, 2017

Wrong error triggered in calls with keyword arguments #688

Closed

jagerman modified the milestone: v2.1 Mar 19, 2017

jagerman mentioned this pull request Dec 15, 2017

Fix leak in var arg handling #1216

Merged

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for positional args with args/kwargs #611

Add support for positional args with args/kwargs #611

jagerman commented Jan 22, 2017

jagerman commented Jan 22, 2017

dean0x7d left a comment

dean0x7d Jan 22, 2017

jagerman Jan 22, 2017

jagerman Jan 22, 2017 •

edited

Loading

dean0x7d Jan 22, 2017

jagerman Jan 22, 2017

dean0x7d Jan 22, 2017

jagerman Jan 22, 2017

dean0x7d Jan 22, 2017

jagerman Jan 23, 2017

jagerman commented Jan 23, 2017

dean0x7d commented Jan 23, 2017

jagerman commented Jan 23, 2017

wjakob commented Jan 31, 2017

Add support for positional args with args/kwargs #611

Add support for positional args with args/kwargs #611

Conversation

jagerman commented Jan 22, 2017

jagerman commented Jan 22, 2017

dean0x7d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman Jan 22, 2017 • edited Loading

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

jagerman commented Jan 23, 2017

dean0x7d commented Jan 23, 2017

jagerman commented Jan 23, 2017

wjakob commented Jan 31, 2017

jagerman Jan 22, 2017 •

edited

Loading