Always display python type information in cast errors #4463

virtuald · 2023-01-18T05:59:04Z

Description

I opened a discussion at #4458:

Currently, the error message is:

RuntimeError: Unable to cast Python instance to C++ type (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)

That's not useful. We should add the python type to this error message to help users out even though we don't have the C++ type. This PR does that.

Suggested changelog entry:

Cast errors now always include Python type information, even if `PYBIND11_DETAILED_ERROR_MESSAGES` is not defined. This increases binary sizes slightly (~1.5%) but the error messages are much more informative.

include/pybind11/cast.h

rwgk · 2023-01-19T01:46:46Z

I'm unsure about this change. My guess: the original idea was to keep the release binaries small, and make the debug behavior comprehensive. This PR waters down the motivation for having the release/debug difference. Questions that cross my mind:

Do we want to accept the binary size increases?
If we do, why not go all the way to the comprehensive messages also for release builds? That would be a nice simplification.

To make that decision scientifically, we'd need some measurements, for typical situations.

Without having data: I feel it's best to either not touch this, or get rid of the release/debug difference altogether.

virtuald · 2023-01-19T02:50:47Z

I don't know the answer to your questions, but:

RuntimeError: Unable to cast Python instance to C++ type is a useless error message. Generally python tries to give good error messages, so should pybind11.
It always seemed to me (without actual knowledge) that the rationale for the debug/release split was to avoid bringing in the C++ type information. The python type information is already there, so it doesn't seem like this would add very much.

virtuald · 2023-01-19T03:51:39Z

I picked one of my packages that has a bunch of manual pybind11 code and typical class/function wrappers. It uses smart_holder, but here are some non-scientific results from that exploration:

pyntcore, Linux gcc 12.2.1, std=c++20, release build

What	Size	%diff	Stripped Size	%diff stripped
smart-holder `8ece7d6`	4318976	0%	2853288	0%
display-python-type-smart-holder `129fc50`	4352464	+0.7%	2886056	+1.5%
smart-holder `8ece7d6` and -DPYBIND11_DETAILED_ERROR_MESSAGES=1	4471904	+3.5%	3000744	+5.1%

rwgk · 2023-01-19T05:36:50Z

here are some non-scientific results from that exploration

Scientific enough for me! That sways my opinion:

+1.5% for a much more useful error message is a good bargain.
The delta to +5.1% is significant enough to still keep the debug/release distinction (IOW the PYBIND11_DETAILED_ERROR_MESSAGES #ifdefs).

I'm assuming the measurements are similar on other platforms. Seems like a pretty safe assumption.

rwgk

Two more suggestions/questions, basically retro-fitting, to improve alignment with best practices:

Change C-style cast to static_cast (or even better IMO str(...).cast<std::string>()) not only in the copy-pasted code, but also in the original. To not create weird asymmetries.
Is all changed code covered by unit tests? I didn't look systematically, but IIUC there are more code changes than test changes. If that's true, now is the right time to plug those holes.

Small worry: this is a behavior change that could break test code. After your pass through my suggestions I'll run global testing to see if that's actually something to worry about.

include/pybind11/detail/common.h

rwgk · 2023-01-19T05:44:51Z

include/pybind11/detail/common.h

@@ -1227,6 +1227,11 @@ constexpr
 // Pybind offers detailed error messages by default for all builts that are debug (through the
 // negation of ndebug). This can also be manually enabled by users, for any builds, through
 // defining PYBIND11_DETAILED_ERROR_MESSAGES.
+//


I'm thinking shorter and purely technical will be best here:

// defining PYBIND11_DETAILED_ERROR_MESSAGES. This will add C++ type information that is otherwise omitted in release builds.

It doesn't just add C++ type information. It also adds some GIL checks (gil.h) and some extra py::arg related checks (attr.h).

The code looks good to me now, thanks.
But the new comment

starts with a sentence that is very similar to the existing sentence above (i.e. does not add new information),

and the "Aid those..." line leaves me puzzled TBH.

It doesn't just add C++ type information. It also adds some GIL checks (gil.h) and some extra py::arg related checks (attr.h).

That's too much detail and is the kind of information that easily goes stale. I'm now thinking it's best to omit the new comment completely.

tests/test_exceptions.cpp

tests/test_exceptions.py

virtuald · 2023-01-24T04:16:56Z

The other cases seem to have test coverage, they were just generic and didn't check the whole exception.

rwgk

LGTM for code changes.

Looking for another review from @Skylion007 or @henryiii

rwgk · 2023-01-24T06:58:42Z

include/pybind11/detail/common.h

@@ -1227,6 +1227,11 @@ constexpr
 // Pybind offers detailed error messages by default for all builts that are debug (through the
 // negation of ndebug). This can also be manually enabled by users, for any builds, through
 // defining PYBIND11_DETAILED_ERROR_MESSAGES.
+//


The code looks good to me now, thanks.
But the new comment

starts with a sentence that is very similar to the existing sentence above (i.e. does not add new information),

and the "Aid those..." line leaves me puzzled TBH.

It doesn't just add C++ type information. It also adds some GIL checks (gil.h) and some extra py::arg related checks (attr.h).

That's too much detail and is the kind of information that easily goes stale. I'm now thinking it's best to omit the new comment completely.

rwgk · 2023-01-24T15:32:11Z

I ran Google global testing for this PR and it passed (internal test ID OCL:504179773:BASE:504260517:1674571457025:b95799a2).

Skylion007

Really tiny nit, but some of these strings could be constructed using std::string("...").append() other strings for a bit more efficiency: https://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html. This looks good either way though.

virtuald · 2023-02-10T04:47:36Z

Comment updated.

rwgk · 2023-02-10T05:21:10Z

Thanks Dustin, I'll merge this now.

Regarding GHA, the Pip failure is expected (currently unresolved: PR #4497), the CI / 🐍 pypy-3.9 • windows-2022 • x64 failure is a long-standing flake.

rwgk · 2023-02-10T05:28:35Z

I added a suggested changelog entry. Feel free to tweak.

Skylion007 reviewed Jan 18, 2023

View reviewed changes

include/pybind11/cast.h Outdated Show resolved Hide resolved

virtuald force-pushed the display-python-type branch from d45fb5b to 5bbc8e2 Compare January 19, 2023 05:00

rwgk reviewed Jan 19, 2023

View reviewed changes

Always display python type information in cast errors

aef70cb

virtuald force-pushed the display-python-type branch from 527148f to 574a6f1 Compare January 24, 2023 04:19

Address comments

10427c8

virtuald force-pushed the display-python-type branch from 574a6f1 to 10427c8 Compare January 24, 2023 04:24

rwgk approved these changes Jan 24, 2023

View reviewed changes

rwgk mentioned this pull request Jan 24, 2023

Tracking test_gil_scoped.py DEADLOCKs #4373

Open

rwgk requested a review from henryiii January 24, 2023 15:32

Skylion007 approved these changes Jan 24, 2023

View reviewed changes

virtuald added 2 commits February 9, 2023 23:45

Merge branch 'master' into display-python-type

9c320f8

Update comment

7a2cfc3

rwgk merged commit 8dcced2 into pybind:master Feb 10, 2023

github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 10, 2023

virtuald deleted the display-python-type branch February 10, 2023 05:24

This was referenced Feb 11, 2023

FWD pybind11 google/pybind11clif#4463

Closed

[smart_holder] git merge master #4512

Merged

henryiii removed the needs changelog Possibly needs a changelog entry label Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always display python type information in cast errors #4463

Always display python type information in cast errors #4463

virtuald commented Jan 18, 2023 •

edited by rwgk

Loading

rwgk commented Jan 19, 2023

virtuald commented Jan 19, 2023

virtuald commented Jan 19, 2023

rwgk commented Jan 19, 2023

rwgk left a comment

rwgk Jan 19, 2023

virtuald Jan 24, 2023

rwgk Jan 24, 2023

virtuald commented Jan 24, 2023

rwgk left a comment

rwgk Jan 24, 2023

rwgk commented Jan 24, 2023

Skylion007 left a comment

virtuald commented Feb 10, 2023

rwgk commented Feb 10, 2023

rwgk commented Feb 10, 2023

Always display python type information in cast errors #4463

Always display python type information in cast errors #4463

Conversation

virtuald commented Jan 18, 2023 • edited by rwgk Loading

Description

Suggested changelog entry:

rwgk commented Jan 19, 2023

virtuald commented Jan 19, 2023

virtuald commented Jan 19, 2023

pyntcore, Linux gcc 12.2.1, std=c++20, release build

rwgk commented Jan 19, 2023

rwgk left a comment

Choose a reason for hiding this comment

rwgk Jan 19, 2023

Choose a reason for hiding this comment

virtuald Jan 24, 2023

Choose a reason for hiding this comment

rwgk Jan 24, 2023

Choose a reason for hiding this comment

virtuald commented Jan 24, 2023

rwgk left a comment

Choose a reason for hiding this comment

rwgk Jan 24, 2023

Choose a reason for hiding this comment

rwgk commented Jan 24, 2023

Skylion007 left a comment

Choose a reason for hiding this comment

virtuald commented Feb 10, 2023

rwgk commented Feb 10, 2023

rwgk commented Feb 10, 2023

virtuald commented Jan 18, 2023 •

edited by rwgk

Loading