Skip to content
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

Experimenting: Annotated[Any, pybind11.CppType("cpp_namespace::UserType")] #4888

Draft
wants to merge 49 commits into
base: master
Choose a base branch
from
Draft

Experimenting: Annotated[Any, pybind11.CppType("cpp_namespace::UserType")] #4888

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
7780fbc
Try `Annotated[Any, "cpp_namespace::UserType"]` unconditionally.
Oct 19, 2023
76b4a34
Add `struct handle_type_name<...>` specializations for `object`, `lis…
Oct 19, 2023
794d97e
Revert "Add `struct handle_type_name<...>` specializations for `objec…
Oct 19, 2023
2cafdab
Add `cpp_name_needs_typing_annotated()`
Oct 19, 2023
f6ae40b
clang-tidy compatibility
Oct 20, 2023
ea00323
Merge branch 'master' into annotated_any
Oct 20, 2023
272152e
`Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]` based on…
Oct 21, 2023
90b3912
Merge branch 'master' into annotated_any
Oct 21, 2023
bb8709a
Remove `cpp_name_needs_typing_annotated()`. Preparation for bringing …
Oct 21, 2023
199532e
Reapply "Add `struct handle_type_name<...>` specializations for `obje…
Oct 21, 2023
7280380
Fix `handle_type_name<anyset>` as suggested by @sizmailov:
Oct 21, 2023
70a510c
Adjust test_numpy_dtypes test_signature to new behavior.
Oct 21, 2023
ace70b0
Render `anyset` as `set | frozenset` as suggested by @sizmailov:
Oct 21, 2023
7c8991a
Use `Union[set, frozenset]` for internal consistency.
Oct 21, 2023
63a4881
Add missing `handle_type_name<slice>` specialization (discovered only…
Oct 22, 2023
3c20944
Add missing `handle_type_name<>` specializations for `bytearray`, `me…
Oct 22, 2023
74817b7
Add missing `handle_type_name<module_>`.
Oct 22, 2023
6a3a954
Add missing `handle_type_name<dtype>`.
Oct 22, 2023
169b0e5
Add test returning `py::exception<void>` (fails at runtime).
Oct 22, 2023
acfd646
Merge branch 'master' into annotated_any
Oct 23, 2023
9548fa5
Merge branch 'master' into annotated_any
Nov 11, 2023
0b98433
Remove `CppTypePybind11()` wrapping for now.
Nov 14, 2023
781304e
Add test_cases_for_stubgen
Nov 14, 2023
61ee34e
Pull in `Annotated[list[int], FixedSize(2)]` from test_stl
Nov 14, 2023
1104076
Add `Annotated[Any, "..."]` wrapping in `type_info_description()`
Nov 14, 2023
e5f210e
Rename `user_type` to `UserType`
Nov 14, 2023
d1694d9
Use locally defined bindings to avoid dependency on test_stl.
Nov 15, 2023
1b4fa71
Unmodified copy of https://github.com/python/mypy/blob/c6cb3c6282003d…
Nov 15, 2023
644d150
Minimal changes to make the basics code build.
Nov 15, 2023
1fa0065
pre-commit clang-format, NO manual changes.
Nov 15, 2023
69dac46
Add `m.basics` tests in ntest_cases_for_stubgen.py
Nov 15, 2023
1a2e8a6
C++11 compatibility.
Nov 15, 2023
79f6bdc
Replace `.stl_binders.` with `.cases_for_stubgen.`
Nov 15, 2023
2376f6e
Use py::handle instead of py::object to avoid clang-tidy errors.
Nov 15, 2023
542438f
Merge branch 'master' into annotated_any
Nov 16, 2023
bdbb10d
Add some deeply nested test cases.
Nov 16, 2023
429a1f8
Make test_cases_for_stubgen.py much more compact, and the `pytest -v`…
Nov 16, 2023
2b2ffeb
Introduce `detail::annotated_any()` helper.
Nov 16, 2023
65661fe
Change `detail::annotated_any()` to produce `pybind11.CppType(...)`
Nov 16, 2023
6b771d5
Merge branch 'master' into annotated_any
Dec 17, 2023
813660c
Change `annotated_any()` to `quote_cpp_type_name()`
Dec 17, 2023
66ee131
Test changes to adjust to previous commit (Change `annotated_any()` t…
Dec 17, 2023
d14d91e
Remove `handle_type_name` default implementation, add explicit specia…
Dec 17, 2023
1e6bea2
Merge branch 'master' into annotated_any
Mar 27, 2024
bf6077a
style: pre-commit fixes
pre-commit-ci[bot] Mar 27, 2024
b02767d
Merge branch 'master' into annotated_any
Mar 27, 2024
67c41cc
Merge branch 'master' into annotated_any
Apr 1, 2024
8c5bb07
Merge branch 'master' into annotated_any
Apr 3, 2024
d57ed51
Adjustments related to pybind/pybind11#4985
Apr 3, 2024
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
6 changes: 5 additions & 1 deletion include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,11 @@ class cpp_function : public function {
} else {
std::string tname(t->name());
detail::clean_type_id(tname);
signature += tname;
if (detail::cpp_name_needs_typing_annotated(tname.c_str())) {
signature += "Annotated[Any, CppTypePybind11(\"" + tname + "\")]";
} else {
signature += tname;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, we should always wrap C++ types with Annotated at this point.
Being a top-level non-template type is not enough to assume 1-to-1 C++-Python type name correspondence, although it is the case for (some) built-in types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the following to cast.h allows us to remove the above condition without turning all tests red.

template <>
struct handle_type_name<str> {
    static constexpr auto name = const_name("str");
};
template <>
struct handle_type_name<set> {
    static constexpr auto name = const_name("set");
};
template <>
struct handle_type_name<anyset> {
    static constexpr auto name = const_name("set");
};
template <>
struct handle_type_name<tuple> {
    static constexpr auto name = const_name("tuple");
};
template <>
struct handle_type_name<frozenset> {
    static constexpr auto name = const_name("frozenset");
};
template <>
struct handle_type_name<list> {
    static constexpr auto name = const_name("list");
};
template <>
struct handle_type_name<dict> {
    static constexpr auto name = const_name("dict");
};
template <>
struct handle_type_name<object> {
    static constexpr auto name = const_name("object");
};

The only two failures probably indicate the error in tests themselves:

Pytest errors 

====================================================================================================================== FAILURES ======================================================================================================================
___________________________________________________________________________________________________________________ test_signature ___________________________________________________________________________________________________________________

doc = <conftest.SanitizedString object at 0x7fafa7b3f8e0>

    def test_signature(doc):
>       assert (
            doc(m.create_rec_nested)
            == "create_rec_nested(arg0: int) -> numpy.ndarray[NestedStruct]"
        )
E       assert --- actual / +++ expected
E         - create_rec_nested(arg0: int) -> numpy.ndarray[Annotated[Any, CppTypePybind11("NestedStruct")]]
E         ?                                               --------------------------------            -- -
E         + create_rec_nested(arg0: int) -> numpy.ndarray[NestedStruct]

doc        = <conftest.SanitizedString object at 0x7fafa7b3f8e0>

../../tests/test_numpy_dtypes.py:344: AssertionError
______________________________________________________________________________________________________________________ test_set ______________________________________________________________________________________________________________________

capture = <conftest.Capture object at 0x7fafa82995a0>, doc = <conftest.SanitizedString object at 0x7fafa82991b0>

    def test_set(capture, doc):
        s = m.get_set()
        assert isinstance(s, set)
        assert s == {"key1", "key2", "key3"}
    
        s.add("key4")
        with capture:
            m.print_anyset(s)
        assert (
            capture.unordered
            == """
            key: key1
            key: key2
            key: key3
            key: key4
        """
        )
    
        m.set_add(s, "key5")
        assert m.anyset_size(s) == 5
    
        m.set_clear(s)
        assert m.anyset_empty(s)
    
        assert not m.anyset_contains(set(), 42)
        assert m.anyset_contains({42}, 42)
        assert m.anyset_contains({"foo"}, "foo")
    
        assert doc(m.get_set) == "get_set() -> set"
>       assert doc(m.print_anyset) == "print_anyset(arg0: anyset) -> None"
E       assert --- actual / +++ expected
E         - print_anyset(arg0: set) -> None
E         + print_anyset(arg0: anyset) -> None
E         ?                    +++

capture    = <conftest.Capture object at 0x7fafa82995a0>
doc        = <conftest.SanitizedString object at 0x7fafa82991b0>
s          = set()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't point out before, I got the same result already, it's in a commit here: 76b4a34

The only remaining errors are related to numpy NestedStruct. I tried to pin-point where the % originates, but I gave up after looking for ~5 minutes, reverted the commit, and then tried the cpp_name_needs_typing_annotated() heuristic instead to see how far we get with that.

I agree what you suggest here is best, but someone needs to figure out how to deal with the failing test, and then we have to decide if we need to roll out the behavior change in stages, maybe opt-in first in one release, then opt-out in the next.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. You already tried that. I should have noticed.

I think we should apply the following diff to tests.

  • NestedStruct doesn't have a descriptive python counterpart, the CppTypePybind11("NestedType") is the best we can do.
  • There is no such thing as anyset in python.
template <>
struct handle_type_name<anyset> {
    static constexpr auto name = const_name("set");
};

This should resolve the troubles with broken tests.

diff 

diff --git a/tests/test_numpy_dtypes.py b/tests/test_numpy_dtypes.py
index d10457ee..2fc3771c 100644
--- a/tests/test_numpy_dtypes.py
+++ b/tests/test_numpy_dtypes.py
@@ -343,7 +343,7 @@ def test_complex_array():
 def test_signature(doc):
     assert (
         doc(m.create_rec_nested)
-        == "create_rec_nested(arg0: int) -> numpy.ndarray[NestedStruct]"
+        == "create_rec_nested(arg0: int) -> numpy.ndarray[Annotated[Any, CppTypePybind11(\"NestedStruct\")]]"
     )
 
 
diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py
index 2b202731..2824d29f 100644
--- a/tests/test_pytypes.py
+++ b/tests/test_pytypes.py
@@ -121,7 +121,7 @@ def test_set(capture, doc):
     assert m.anyset_contains({"foo"}, "foo")
 
     assert doc(m.get_set) == "get_set() -> set"
-    assert doc(m.print_anyset) == "print_anyset(arg0: anyset) -> None"
+    assert doc(m.print_anyset) == "print_anyset(arg0: set) -> None"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NestedStruct doesn't have a descriptive python counterpart

Nice, thanks! I didn't know / wasn't sure before. I'm glad I gave up before, finding out the hard way. :-)

There is no such thing as anyset in python.

Of course! :-) Thanks for catching that as well, I was just going through with rushed lawnmower mentality.

Please take another look, the cpp_name_needs_typing_annotated() heuristic is gone, all tests pass (locally at least).

}
} else {
signature += c;
Expand Down
10 changes: 10 additions & 0 deletions include/pybind11/typing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,5 +113,15 @@ struct handle_type_name<typing::Callable<Return(Args...)>> {
+ const_name("]");
};

inline bool cpp_name_needs_typing_annotated(const char *cpp_name) {
while (*cpp_name != '\0') {
char c = *cpp_name++;
if (c == ':' || c == '<') { // Assuming valid names, there is no need to check for '>'.
return true;
}
}
return false;
}

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)