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

Keyword arguments and generalized unpacking for C++ API #372

Merged
merged 10 commits into from Sep 6, 2016

Conversation

Projects
None yet
4 participants
@dean0x7d
Copy link
Member

dean0x7d commented Aug 29, 2016

General idea

A Python function can be called with the syntax:

foo(a1, a2, *args, ka=1, kb=2, **kwargs)

This PR adds support for the equivalent syntax in C++:

foo(a1, a2, *args, "ka"_a=1, "kb"_a=2, **kwargs)

In addition, generalized unpacking is implemented, as per PEP 448, which allows calls with multiple * and ** unpackings in Python:

bar(*args1, 99, *args2, 101, **kwargs1, kz=200, **kwargs2)

and in C++:

bar(*args1, 99, *args2, 101, **kwargs1, "kz"_a=200, **kwargs2)

Impact on compile time

The new functionality is SFINAEd-off into a separate instantiation path from the existing positional-only arguments. Thus, the compile time will not increase for existing code. When (and only when) keywords and/or generalized unpacking are used, a little more template metaprogramming is in play, but it's not really any more complicated than the existing def/arg machinery for naming arguments and assigning default values. There are no compile time slowdowns as far as I've seen.

Python version compatibility

PEP 448 is implemented only in Python >= 3.5, but the C++ API allows these kinds of calls for any Python version. Limiting it by version is also possible, but I don't think it's necessary. The function calls are still perfectly valid, it's just that:

f(**kwargs1, **kwargs2)  # Python 3.5

would look like:

kwargs1.update(kwargs2)  # Python < 3.5
f(**kwargs1)

Applications

This PR also implements a few functions using this new functionality.

Python's print function is replicated in the C++ API including the keyword arguments sep, end, file, flush. E.g.:

py::print(v1, v2, "sep"_a=" -- ");

The str.format method is added. Together with the _s UDL for str, this allows the following syntax in C++:

auto str1 = "Hello, {}! Your number is {}"_s.format("World", 42);
// or
auto str2 = "Hello, {name}! Your number is {number}"_s.format("number"_a=42, "name"_a="World");

The py::dict class also gets a keyword constructor which replicates its Python counterpart:

auto d = dict("number"_a=42, "name"_a="World");

Feedback?

I hope this is not considered too much of an abuse of C++. Let me know if this is OK so far.

@@ -308,6 +309,7 @@ template <typename type> class type_caster_base : public type_caster_generic {
};

template <typename type, typename SFINAE = void> class type_caster : public type_caster_base<type> { };
template <typename type> using make_caster = type_caster<intrinsic_t<type>>;

This comment has been minimized.

@wjakob

wjakob Aug 30, 2016

Member

this type alias is a good idea -- should have added that much earlier :)

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

For consistency, it would be nice to this make_caster alias in all places where type_caster<typename intrinsic_type<type>::type> is currently used (quite a few I think)

>;

/// Implementation details for constexpr functions
NAMESPACE_BEGIN(constexpr_impl)

This comment has been minimized.

@wjakob

wjakob Aug 30, 2016

Member

extra sub-namespace for (implementation detail)^2 can probably be collapsed into just "detail"

This comment has been minimized.

@wjakob

wjakob Aug 30, 2016

Member

well, never mind. Both work for me

This comment has been minimized.

@dean0x7d

dean0x7d Aug 30, 2016

Author Member

I was going back and forth, whether to add the extra namespace. In the end I just wanted to avoid needlessly long names like constexpr_first_impl (which end up being repeated a few time because of the recursion).

@wjakob

This comment has been minimized.

Copy link
Member

wjakob commented Aug 30, 2016

After a brief look, this seems like very a nice idea (and I like the print function). I'll have to set a aside a bit more time to look at all the metatemplate code but for now it seems very reasonable.

Is there a symmetrical change that would also have to be done to function call handlers, or are these feature-complete as far as PEP 448 goes?

@dean0x7d

This comment has been minimized.

Copy link
Member Author

dean0x7d commented Aug 30, 2016

Is there a symmetrical change that would also have to be done to function call handlers, or are these feature-complete as far as PEP 448 goes?

As far as I can tell, everything is already fine in the other direction. Generalized unpacking is just syntax sugar for the interpreter. The multiple tuples/dicts are merged and the C API only sees the usual pair of args/kwargs.

@aldanor

This comment has been minimized.

Copy link
Member

aldanor commented Aug 30, 2016

This is pretty cool. A few lines in the changelog would be nice

@dean0x7d dean0x7d force-pushed the dean0x7d:keywords branch from 7a8596a to 1f13b0c Sep 4, 2016

@dean0x7d

This comment has been minimized.

Copy link
Member Author

dean0x7d commented Sep 4, 2016

Updated docs with the new call syntax. Updated changelog with all the new features.
The py::dict class also got a keyword constructor which replicates its Python counterpart:

auto d = dict("number"_a=42, "name"_a="World");

MSVC had an issue which required a slightly ugly workaround. It's not too bad, but perhaps somebody has a better solution.

@dean0x7d dean0x7d changed the title WIP: Keyword arguments and generalized unpacking for C++ API Keyword arguments and generalized unpacking for C++ API Sep 4, 2016


const char *name;
};

/// Annotation for keyword arguments with default values
template <typename T> struct arg_t : public arg {
constexpr arg_t(const char *name, const T &value, const char *descr = nullptr)
: arg(name), value(value), descr(descr) { }
T value;
: arg(name), value(&value), descr(descr) { }

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

Hmm, this change (pointer instead of an explicit copy of value) seems problematic to me. It means that arg_t cannot receive something that is constructed on the fly, e.g.

m.def("fun", py::arg("arg") = MyArg(1, 2, 3));

This comment has been minimized.

@dean0x7d

dean0x7d Sep 5, 2016

Author Member

Temporary objects (even ones which are not bound to a const&) are only destroyed after the full expression is evaluated. A full expression is one which is not contained within any other expression (so essentially, temp object lifetime is extended to a semicolon).

This makes the following line perfectly safe:

f("arg"_a=MyArg(1, 2, 3));

On the other hand,

auto a = "arg"_a=MyArg(1, 2, 3);
f(a);

would not be safe, but this is currently a compile time error because unpacking_collector only accepts rvalues: arg<T>&&. (Although, I do need to add this safeguard to process_attribute as well.)

That said, I think the ideal implementation of arg_t would be to hold a py::object instead of T:

struct arg_t : arg {
    template <typename T>
    arg_t(const char *name, const T &value, const char *descr = nullptr)
        : arg(name), value(make_caster<T>::cast(value, return_value_policy::automatic)), descr(descr) { }
    object value;
    const char *descr;
}

The to-python cast would then be done right away instead of being delayed. I didn't make this change originally because default arguments are collected with the automatic policy while function calls use automatic_reference by default. Would there be any downside to always using automatic for call argument collection? (Users should be able to override by doing a manual py::cast(value, policy) when assigning an argument.)

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

Right -- that makes sense. Is the restriction to rvalues unavoidable? If it's easy to also support your example

auto a = "arg"_a=MyArg(1, 2, 3); f(a);

with minor changes then that would be nice. Switching to py::object for the underlying argument storage in this case is fine to me. The only reason for the current state of affairs is because of the way in which def() arguments were processed previously.

Regarding value policies: this unlikely to be a serious concern for default arguments constructed at module load time, and it's always safe to make a copy (which is what will happen for const T& instances that have not been previously observed by pybind11).

This comment has been minimized.

@dean0x7d

dean0x7d Sep 5, 2016

Author Member

Switching to py::object would be the best case. There would be no restrictions on py::arg_t then. As a bonus, the template code would also be simplified since py::arg_t would no longer be a template class (it would only have a template constructor). py::arg_t should create it's py::object using the automatic policy which looks like the only reasonable choice.

There is just one small inconsistency that this would introduce: Keyword arguments would then be passed to function calls with the automatic policy, while positional arguments would use automatic_reference (which is the function call default). I don't think this a big issue, but maybe you have some thoughts.

This comment has been minimized.

@wjakob

wjakob Sep 6, 2016

Member

I don't think it is a big issue. In any case, the cast to a py::object could also be done manually with any kind of policy, which is then assigned to py:arg_t.

@@ -246,7 +241,7 @@ struct process_attribute<arg_t<T>> : process_attribute_default<arg_t<T>> {

/* Convert keyword value into a Python object */
object o = object(detail::type_caster<typename detail::intrinsic_type<T>::type>::cast(
a.value, return_value_policy::automatic, handle()), false);
*a.value, return_value_policy::automatic, handle()), false);

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

related to the above

template <template<typename> class Predicate, typename... Ts>
constexpr int constexpr_last() { return constexpr_impl::last(0, -1, Predicate<Ts>::value...); }

/// Collect only positional arguments

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

A bit longer comment? E.g. "Helper class which collects [...] when performing a function call. A fancier version below also does [..]"

};

/// Collect positional, keyword, * and ** arguments
template <return_value_policy policy>

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

Cool -- the advanced unpacking collector implementation very elegant and easy to understand. 👍

unpacking_collector<policy> collect_arguments(Args &&...args) {
// Following argument order rules for generalized unpacking according to PEP 448
static_assert(
constexpr_last<is_positional, Args...>() < constexpr_first<is_keyword_or_ds, Args...>()

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

nice :)

};

inline namespace literals {

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

TIL about inline namespaces

size_t size() const { return (size_t) PyDict_Size(m_ptr); }
detail::dict_iterator begin() const { return (++detail::dict_iterator(*this, 0)); }
detail::dict_iterator end() const { return detail::dict_iterator(); }
void clear() const { PyDict_Clear(ptr()); }
object get(const char* key, handle def_value = none()) const {

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

This change seems unrelated to this PR -- perhaps it's better to discuss separately.


def _flush_stdout(self):
def _flush(self):
sys.stdout.flush()

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

These double flushes are somewhat surprising, I assume it's to work around bugs. Care to write a comment about it?

This comment has been minimized.

@dean0x7d

dean0x7d Sep 5, 2016

Author Member

The extra effort was mostly to work around issues on Windows. After this I plan to submit a PR which replaces std::cout with py::print everywhere, which fixes all output issues on Windows. At that point this entire _flush() method will be deleted since pytest's capfd already takes care of it.

@@ -1589,24 +1589,56 @@ It is also possible to call python functions via ``operator()``.
py::object result_py = f(1234, "hello", some_instance);

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

py::print not mentioned in docs (except changelog)

This comment has been minimized.

@dean0x7d

dean0x7d Sep 5, 2016

Author Member

What do you think about creating a new page dedicated to the Python C++ API? The wrapper classes (str, tuple, dict) are also a bit underdocumented, but the the Advanced page is already getting quite long and hard to navigate. (It's probably worth splitting of additional sections which form a logical unit (like buffer/numpy/eigen) but that's a topic for a different PR.)

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

This sounds good to me.

Sent from my iPhone

On 05 Sep 2016, at 18:06, Dean Moldovan notifications@github.com wrote:

In docs/advanced.rst:

@@ -1589,24 +1589,56 @@ It is also possible to call python functions via operator().
py::object result_py = f(1234, "hello", some_instance);
What do you think about creating a new page dedicated to the Python C++ API? The wrapper classes (str, tuple, dict) are also a bit underdocumented, but the the Advanced page is already getting quite long and hard to navigate. (It's probably worth splitting of additional sections which form a logical unit (like buffer/numpy/eigen) but that's a topic for a different PR.)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/pybind/pybind11","title":"pybind/pybind11","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/pybind/pybind11"}},"updates":{"snippets":[{"icon":"PERSON","message":"@dean0x7d in #372: What do you think about creating a new page dedicated to the Python C++ API? The wrapper classes (str, tuple, dict) are also a bit underdocumented, but the the Advanced page is already getting quite long and hard to navigate. (It's probably worth splitting of additional sections which form a logical unit (like buffer/numpy/eigen) but that's a topic for a different PR.)"}],"action":{"name":"View Pull Request","url":"https://github.com/pybind/pybind11/pull/372/files/1f13b0c69e27b520b319fdcc170e0e52fb957644#r77490851"}}}

This comment has been minimized.

@aldanor

aldanor Sep 5, 2016

Member

Yea, agreed, most of this library now falls under "Advanced", and things like objects in pytypes.h are scarcely mentioned.

Splitting into sections could be good -- eg wrapper types (dict, tuple etc), handle/object, utility (print), eigen, NumPy (I'd be willing to rewrite the docs on this one).

This comment has been minimized.

@wjakob

wjakob Sep 5, 2016

Member

Pybind11 has three different ways of passing values between C++ and Python. Custom C++ classes mapped to Python (class_<>, convenience wrappers around it like stl_binder.h), Python type wrappers (py::object, py::array, etc.), and conversion type casters which convert C++ types into equivalent Python types and vice versa while creating copies in the process (for int, std::string, all stuff in stl.h, eigen.h). I was thinking that it might make sense to split the advanced documentation into three different parts corresponding to each. Thoughts?

@wjakob

This comment has been minimized.

Copy link
Member

wjakob commented Sep 5, 2016

I think this one is almost ready to go. I added a few comments. The only major gotcha is the pointer-vs-value storage in arg_t, can you take a look at that? Maybe I am just missing something though.

/// Utility types for metaprogramming
template <bool...> struct bools { };
template <typename...> struct always_true : std::true_type { };
template <typename...> struct always_false : std::false_type { };

This comment has been minimized.

@jagerman

jagerman Sep 5, 2016

Member

Could these be written as: template <typename...> using always_true = std::true_type; and likewise for always_false?

This comment has been minimized.

@dean0x7d

dean0x7d Sep 5, 2016

Author Member

Yes, that was the initial implementation, however GCC 4.8 had an issue with it in one spot (which is weird since even MSVC compiled it correctly). I'll need to investigate a bit more to see what's up.

@wjakob

This comment has been minimized.

Copy link
Member

wjakob commented Sep 6, 2016

Let me know when you think this is ready to merge.

dean0x7d added some commits Aug 28, 2016

Support keyword arguments and generalized unpacking in C++
A Python function can be called with the syntax:
```python
foo(a1, a2, *args, ka=1, kb=2, **kwargs)
```
This commit adds support for the equivalent syntax in C++:
```c++
foo(a1, a2, *args, "ka"_a=1, "kb"_a=2, **kwargs)
```

In addition, generalized unpacking is implemented, as per PEP 448,
which allows calls with multiple * and ** unpacking:
```python
bar(*args1, 99, *args2, 101, **kwargs1, kz=200, **kwargs2)
```
and
```c++
bar(*args1, 99, *args2, 101, **kwargs1, "kz"_a=200, **kwargs2)
```
Add py::print() function
Replicates Python API including keyword arguments.
Remove superseded handle::operator() overloads
The variadic handle::operator() offers the same functionality as well
as mixed positional, keyword, * and ** arguments. The tests are also
superseded by the ones in `test_callbacks`.
Workaround for py::dict() constructor on MSVC
MSVC fails to compile if the constructor is defined out-of-line.
The error states that it cannot deduce the type of the default template
parameter which is used for SFINAE.

dean0x7d added some commits Sep 5, 2016

Make keyword argument hold a py::object instead of T*
With this change arg_t is no longer a template, but it must remain so
for backward compatibility. Thus, a non-template arg_v is introduced,
while a dummy template alias arg_t is there to keep old code from
breaking. This can be remove in the next major release.

The implementation of arg_v also needed to be placed a little earlier in
the headers because it's not a template any more and unpacking_collector
needs more than a forward declaration.

@dean0x7d dean0x7d force-pushed the dean0x7d:keywords branch from 1f13b0c to 60b2680 Sep 6, 2016

@dean0x7d

This comment has been minimized.

Copy link
Member Author

dean0x7d commented Sep 6, 2016

OK, if everything looks good with the changes to py::arg this should be ready to go. See the last commit message for py::arg details.

For the docs, I added a quick section for py::print, but everything is still in Advanced. I think the reorganization is better left for a separate PR. Better docs for the type wrappers can also be added at that point.

I addressed all the comments and squashed the smaller changes (remove dict.get, improved comments) with their respective commits to keep the git history relatively tidy.

template <typename T>
arg_v arg::operator=(T &&value) const { return {name, std::forward<T>(value)}; }

/// Alias for backward compatibility -- to be remove in version 2.0

This comment has been minimized.

@wjakob

wjakob Sep 6, 2016

Member

very compact 👍

@wjakob

This comment has been minimized.

Copy link
Member

wjakob commented Sep 6, 2016

Awesome, merged! 🎉

@wjakob wjakob merged commit a3dbdc6 into pybind:master Sep 6, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dean0x7d dean0x7d deleted the dean0x7d:keywords branch Sep 6, 2016

@dean0x7d dean0x7d referenced this pull request Oct 15, 2016

Closed

Make a v2.0 release? #442

6 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment