Allow kw-only args after a py::args

[jagerman]

Description

This removes the constraint that py::args has to be last (or second-last, with py::kwargs) and instead makes py::args imply py::kw_only for any remaining arguments, allowing you to bind a function that works the same way as a Python function such as:

def f(a, *args, b):
    return a * b + sum(args)

f(10, 1, 2, 3, b=20)  # == 206

With this PR you can now bind such a function using pybind11:

    m.def("f", [](int a, py::args args, int b) { /* ... */ },
        "a"_a, "b"_a);

Or, to be more explicit about the keyword-only arguments:

    m.def("g", [](int a, py::args args, int b) { /* ... */ },
        "a"_a, py::kw_only{}, "b"_a);

The only difference between the two is that the latter will fail at binding time if the py::kw_only{} position doesn't match the relative position of the py::args argument.

(This doesn't affect backwards compatibility at all because, currently, you can't have a py::args anywhere except the end/2nd-last.)

Suggested changelog entry:

* Allow ``py::args`` to be followed by other arguments; the remaining
  arguments are implicitly keyword-only, as if a `py::kw_only{}` annotation
  had been used.

[Skylion007]

These tests should all pass py::args and py::kwargs by ref through the lambda since they are not trivially copyable. (This is the clang-tidy rule that fails).

[henryiii]

Let's try to get the last few fixes in to 2.8.1, then merge this (also okay to branch, but since we are very close to a patch release, a few day delay is probably fine). Other than the tiny test fix, this sounds like a great idea.

[Skylion007]

It surprises me that py::args is not trivially copyable.

[henryiii]

py::args is a py::tuple, which is a py::object, and copying a py::object increases the reference count, so it's not trivially copyable.

[henryiii]

Thanks for doing this! It was something I thought about briefly when adding pos_only and touching up kw_only, but never came back to.

You've mixed MD and RST syntax in the changelog entry above. ;) I think we need to bite the bullet and make the switch to MD for the changelog. Soon.

[jagerman]

py::args is a py::tuple, which is a py::object, and copying a py::object increases the reference count, so it's not trivially copyable.

Though considering that in order to touch any of this you have to hold the GIL, and thus the reference count doesn't even have to be atomic, it's fairly "trivial" in practice, just not in the C++ sense of the word.

[rwgk]

Just a quick note at the moment (I need to find time to debug): I'm seeing test breakages in Google's global testing that may be related to this PR.

https://github.com/google/tensorstore

TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. tensorstore.IndexTransform(input_rank: Optional[int] = None, *, input_inclusive_min: Optional[Sequence[int]] = None, implicit_lower_bounds: Optional[Sequence[bool]] = None, input_exclusive_max: Optional[Sequence[int]] = None, input_inclusive_max: Optional[Sequence[int]] = None, input_shape: Optional[Sequence[int]] = None, implicit_upper_bounds: Optional[Sequence[bool]] = None, input_labels: Optional[Sequence[Optional[str]]] = None, output: Optional[Sequence[tensorstore.OutputIndexMap]] = None)
    2. tensorstore.IndexTransform(domain: tensorstore.IndexDomain, output: Optional[Sequence[tensorstore.OutputIndexMap]] = None)
    3. (*, self: tensorstore.IndexTransform, json: Any) -> None

Invoked with: kwargs: json={'input_exclusive_max': [2, 3], 'input_inclusive_min': [0, 0], 'input_labels': ['x', 'y']}
def test_translate_by_vector():
      x = tstf.IndexTransform(input_shape=[2, 3], input_labels=["x", "y"])
>     check_dim_expr(x, lambda d: d["x", "y"].translate_by[4, 5])

third_party/py/tensorstore/tensorflow/tests/dim_expression_test.py:54: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
third_party/py/tensorstore/tensorflow/tests/dim_expression_test.py:43: in check_dim_expr
    ts_transform = transform.to_pyval()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[TypeError("__init__(): incompatible constructor arguments. The following argument types are supported:\n    1. tenso...'input_inclusive_min': [0, 0], 'input_labels': ['x', 'y']}") raised in repr()] IndexTransform object at 0x7f03a893db90>

    def to_pyval(self) -> tensorstore.IndexTransform:
      """Returns an equivalent `tensorstore.IndexTransform.
    
      This is only supported under eager execution mode.
      """
      if not tf.executing_eagerly():
        raise NotImplementedError(
            'IndexTransform.to_pyval only supported during eager execution')
>     return tensorstore.IndexTransform(json=self.to_json())
E     TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
E         1. tensorstore.IndexTransform(input_rank: Optional[int] = None, *, input_inclusive_min: Optional[Sequence[int]] = None, implicit_lower_bounds: Optional[Sequence[bool]] = None, input_exclusive_max: Optional[Sequence[int]] = None, input_inclusive_max: Optional[Sequence[int]] = None, input_shape: Optional[Sequence[int]] = None, implicit_upper_bounds: Optional[Sequence[bool]] = None, input_labels: Optional[Sequence[Optional[str]]] = None, output: Optional[Sequence[tensorstore.OutputIndexMap]] = None)
E         2. tensorstore.IndexTransform(domain: tensorstore.IndexDomain, output: Optional[Sequence[tensorstore.OutputIndexMap]] = None)
E         3. (*, self: tensorstore.IndexTransform, json: Any) -> None
E     
E     Invoked with: kwargs: json={'input_exclusive_max': [2, 3], 'input_inclusive_min': [0, 0], 'input_labels': ['x', 'y']}

third_party/py/tensorstore/tensorflow/index_transform.py:926: TypeError

[henryiii]

Here's where the defs are: https://github.com/google/tensorstore/blob/1a59fcb310bc1feb13569f03f7134b4c3a5fa5f4/python/tensorstore/index_space.cc#L823

My guess is the logic for computing these is off (the *, self looks really off to me), and we don't have a properly complex test case for mixing these types of args. Just a guess, though.

[rwgk]

Hi @jagerman, do you think you'll have time to look into this soon?

As-is, I'm blocked from updating pybind11 for Google-internal use (smart_holder branch), which could turn into a secondary problem for OpenSpiel, which is using the smart_holder branch internally and externally.