Handling warnings from C++ #4535

jiwaszki · 2023-02-26T23:39:25Z

Description

Providing a simple interface to handle warnings on the pybind side. Following up #601 issue.

New py::warning class that provide wrapper for custom warnings.
Added py::warnings namespace containing shorthand versions of Python C API warning categories.
Added raise_warning that allow to use warnings directly from py::warnings namespace or created by the user.

PS It is my first time contributing something "bigger" (finally! :)). Please, look at "TODO" section, there are some high-level decisions to be made. These are ones I have spotted. All reviews are welcome!

TODO:

Which order of arguments is better?

// First option - allows to default category, follows Python's warnings.warn convention
raise_warning(const char *message, handle category = warnings::runtime, ssize_t stack_level = 2);

// Second option - force to choose category, follows PyErr_WarnEx convention
raise_warning(handle category, const char *message, ssize_t stack_level = 2);

Which stack_level value is more relevant? C API is not defining default stack_level. According to Python docs on warnings.warn the default value is 1. However, there is also a good point on the default to be set to 2:

This makes the warning refer to deprecation()’s caller, rather than to the source of deprecation() itself (since the latter would defeat the purpose of the warning message).

Is py::warnings namespace a good name? Maybe py::warning_categories is more descriptive and not conflicting with py::warning? ... or should it be removed entirely?
Documentation entry (Let's move with this PR first. Is it possible to handle docs in a separate PR?).

Suggested changelog entry:

Added `py::warning` class, `py::warnings` namespace and `py::raise_warning()` which provide the interface for Python warnings.

EthanSteinberg

Looks like a nice feature! Just a couple of comments.

EthanSteinberg · 2023-03-02T23:12:39Z

include/pybind11/pytypes.h

+    if (result == 1) {
+        return true;
+    }
+    if (result == -1) {


We should probably have a more visible error / warning here right? Isn't -1 from IsSubclass supposed to be propagated to users?

Is new commit OK? I am just not sure what "guidelines" on errors are applied for this specific scenario.

EthanSteinberg · 2023-03-02T23:14:22Z

include/pybind11/pytypes.h

+PYBIND11_NAMESPACE_BEGIN(warnings)
+
+// Warning class
+static PyObject *warning_base = PyExc_Warning;


Why aren't these const?

Or even constexpr

Good point. However, from what I understand constexpr won't work out of the box for non-const pointers. Changed to consts.

EthanSteinberg · 2023-03-02T23:20:12Z

tests/test_warnings.py

+    assert m.CustomWarning is not None
+    assert issubclass(m.CustomWarning, DeprecationWarning)
+
+    import warnings


Why isn't this import at the top of the file?

It was put there as local import for this specific test. Sure it can be moved around! Done.

EthanSteinberg

I think there was a bit of a misunderstanding. You made those pointer to constants (which is incorrect, as shown by those incorrect const casts) instead of constant pointers.

See

https://www.google.com/amp/s/www.geeksforgeeks.org/difference-between-constant-pointer-pointers-to-constant-and-constant-pointers-to-constants/amp/

EthanSteinberg · 2023-03-18T15:33:50Z

include/pybind11/pybind11.h

+                      "PyExc_Warning!");
+    }
+
+    if (PyErr_WarnEx(const_cast<PyObject *>(category), message, stack_level) == -1) {


These const casts aren't safe

…_wrappers

jiwaszki · 2024-07-17T17:08:29Z

Just pinging for a review @Skylion007 and @henryiii - sorry for bothering and really long time to update this PR.

rwgk · 2024-07-18T00:45:19Z

include/pybind11/pybind11.h

+/**
+ * Wrapper to generate a new Python warning type.
+ *
+ * It is not (yet) possible to use PyObject as a py::base.


IMO the existing exception type is a failed design that is better not replicated here. All it does is wrap a PyErr_NewException() call, without any connection to the "reserved" template type. It's more likely to be misleading/confusing than helpful. E.g. anyone uninitiated reading client code like this ...

https://github.com/pybind/pybind11_abseil/blob/32cf30f118708025a92456d076cb15a73c7bea73/pybind11_abseil/status_utils.cc#L91

... would think there is a formal connection between the C++ exception type and the Python exception type, but there is none here. That connection is (maybe) only established elsewhere, with an exception translator.

I think what we need here is something simpler and straightforward, for example:

PYBIND11_NAMESPACE_BEGIN(warnings) inline object new_warning_type(handle scope, const char *name, handle base = PyExc_RuntimeWarning) { /* similar to what you have below */ } // Similar to Python `warnings.warn()` inline void warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) { /* similar to what you have below */ } PYBIND11_NAMESPACE_END(warnings)

Could you please try that?

I think it's best implemented in a new pybind11/warnings.h file.

That's all I'd do. I definitely wouldn't want to introduce a bunch of aliases (warning_base, bytes, deprections, ...), which are essentially just a level of obfuscation: people would have to learn ah, it's just PyExc_BytesWarning etc.

I recommend closing this PR and starting a new one.

I see the point. I have already opened new PR for it, let's continue there.

rwgk · 2024-07-18T00:52:35Z

include/pybind11/pytypes.h

+        return true;
+    }
+    if (result == -1) {
+        PyErr_Clear();


Never discard an error only to replace it with something more opaque: people will want to know what the "internal error" is (line below).

Best here: use raise_from() followed by throw error_already_set().

rwgk · 2024-07-18T01:06:01Z

tests/test_warnings.cpp

+          []() { py::raise_warning("RuntimeError should be raised!", PyExc_Exception); });
+
+    // Test custom warnings
+    static py::warning<CustomWarning> my_warning(m, "CustomWarning", py::warnings::deprecation);


This is a very common pitfall:

The destructor may run (and call Py_DECREF) only after the Python interpreter is finalized already.

Best to adopt this approach:

pybind11/tests/test_exceptions.cpp

Lines 119 to 121 in 43de801

PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;

ex_storage.call_once_and_store_result(

[&]() { return py::exception<MyException>(m, "MyException"); });

Warnings wrappers, adding warning class and raise_warning()

3f54c50

jiwaszki requested a review from henryiii as a code owner February 26, 2023 23:39

style: pre-commit fixes

ed16550

EthanSteinberg suggested changes Mar 2, 2023

View reviewed changes

jiwaszki and others added 3 commits March 17, 2023 00:44

Adjust to const, apply comments

78235ac

style: pre-commit fixes

ad9eaf1

Fix for ssize_t/int conversion

0cc1c3e

jiwaszki requested review from Skylion007 and EthanSteinberg and removed request for henryiii and Skylion007 March 17, 2023 08:29

EthanSteinberg suggested changes Mar 18, 2023

View reviewed changes

Change to constant pointers

3dc83a4

N-Wouda mentioned this pull request Jun 25, 2024

MAX_VALUE checks for tw_early, service_duration PyVRP/PyVRP#599

Open

jiwaszki and others added 3 commits July 15, 2024 01:09

Merge remote-tracking branch 'upstream/master' into jiwaszki/add_warn…

60314d5

…_wrappers

style: pre-commit fixes

0dfebe1

Remove unused imports and add stacklevel to warnings

d754b47

jiwaszki requested review from EthanSteinberg and Skylion007 July 15, 2024 00:08

rwgk reviewed Jul 18, 2024

View reviewed changes

jiwaszki mentioned this pull request Aug 8, 2024

feat: warnings wrappers to use from C++ #5291

Merged

jiwaszki closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling warnings from C++ #4535

Handling warnings from C++ #4535

jiwaszki commented Feb 26, 2023

EthanSteinberg left a comment

EthanSteinberg Mar 2, 2023

jiwaszki Mar 16, 2023

EthanSteinberg Mar 2, 2023

Skylion007 Mar 10, 2023

jiwaszki Mar 16, 2023

EthanSteinberg Mar 2, 2023

jiwaszki Mar 16, 2023

EthanSteinberg left a comment

EthanSteinberg Mar 18, 2023

jiwaszki commented Jul 17, 2024

rwgk Jul 18, 2024

jiwaszki Aug 8, 2024

rwgk Jul 18, 2024

rwgk Jul 18, 2024

	PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
	ex_storage.call_once_and_store_result(
	[&]() { return py::exception<MyException>(m, "MyException"); });

Handling warnings from C++ #4535

Handling warnings from C++ #4535

Conversation

jiwaszki commented Feb 26, 2023

Description

TODO:

Suggested changelog entry:

EthanSteinberg left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EthanSteinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiwaszki commented Jul 17, 2024

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