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

jiwaszki · 2024-08-08T23:41:55Z

Description

Providing a simple interface to handle warnings on the pybind side, using Python's warning categories. Following up #601 issue and "re-do" of #4535 PR.

Added py::warnings namespace.
Added warnings::warn that allow to throw warnings directly.
Added warnings:: new_warning_type to create new warning type on specific pybind's scope.

Suggested changelog entry:

Added ``py::warnings`` namespace with ``py::warnings::warn`` and ``py::warnings::new_warning_type`` that provides the interface for Python warnings.

jiwaszki · 2024-08-08T23:43:52Z

@rwgk this is continuation of #4535. Hope that all issues are addressed properly, awaiting your review!

rwgk

Looks great, I think this is a useful addition.

All comments are minor.

You need to add the new header in one more file. A nice trick to pin-point where is to look for an existing header, e.g.:

$ git grep include/pybind11/typing.h
CMakeLists.txt:    include/pybind11/typing.h)
tests/extra_python_package/test_files.py:    "include/pybind11/typing.h",

I didn't review the tests yet (it's very late in my timezone). I'll do that in the next round.

rwgk · 2024-08-11T06:05:27Z

include/pybind11/warnings.h

+                      + std::string(name) + "\"");
+    }
+    std::string full_name = scope.attr("__name__").cast<std::string>() + std::string(".") + name;
+    handle h(PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), nullptr));


I believe the PyErr_NewException() documentation is incomplete because there is no mention of errors, but almost certainly we need at least this:

if (!h) { throw error_already_set(); }

I think that'd be sufficient, although raise_from() like you have above might be even better. I'm not sure how to judge cost:benefit here.

I agree on that, I have modified it and move creation of handle after the check -- so any null should be verified first.

About the docs... yes, they are "very vague" about error messages so referring to them and cpython code this is the best that can be done. However, I am not able to think of a way of testing it. Seems like there is no way to break it. To break it down:

name is always consisting one dot - it's forced by full_name.

base is also validated by PyWarning_Check (plus it could also be null by design of cpython function).

dict by itself is always a nullptr here so that check in cpython is automatically "passed".

I can only imagine if somehow modulename is broken. Maybe I am missing something but it seems to me like trying to break just to prove "that it's breakable". Do you have any thoughts on that?

It's very common that testing of error conditions is incomplete. It's often just too difficult.

What I usually do: temporarily change e.g. if (ptr == nullptr) to if (ptr != nullptr) so that the if branch is reached, run the tests to convince myself that it works as intended, then change it back.

rwgk · 2024-08-11T06:07:57Z

include/pybind11/warnings.h

+    }
+    if (result == -1) {
+        raise_from(PyExc_SystemError,
+                   "PyWarning_Check(): internal error of Python C API while "


Maybe a little simpler?

raise_from(PyExc_SystemError, "pybind11::detail::PyWarning_Check(): PyObject_IsSubclass() call failed.");

I think the fully-qualified function name is important here, otherwise people will think is a C Python function.

I see the point, applied.

rwgk · 2024-08-11T06:09:05Z

include/pybind11/warnings.h

+inline object
+new_warning_type(handle scope, const char *name, handle base = PyExc_RuntimeWarning) {
+    if (!detail::PyWarning_Check(base.ptr())) {
+        pybind11_fail("warning(): cannot create custom warning, base must be a subclass of "


pybind11_fail("warnings::new_warning_type(): ...")

?

To make it easier to pin-point this code based on the error message.

Applied, also I have followed the pybind11::* convention from #5291 (comment) . I think it's better to keep it uniform across all messages, what do you think?

rwgk · 2024-08-11T06:14:13Z

include/pybind11/warnings.h

+                      "PyExc_Warning!");
+    }
+    if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name)) {
+        pybind11_fail("Error during initialization: multiple incompatible "


Maybe

pybind11_fail("new_warning_type(): an attribute with name \"" + std::string(name) + "\" exists already.");

?

I'm also wondering, could the condition simply be hasattr(scope, name)? (I'm really not sure.)

You are right, it works the same. Also added test case for it.

For the message same as #5291 (comment)

rwgk · 2024-08-11T06:15:11Z

include/pybind11/warnings.h

+    }
+    std::string full_name = scope.attr("__name__").cast<std::string>() + std::string(".") + name;
+    handle h(PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), nullptr));
+    object obj = reinterpret_steal<object>(h);


I saw clang-tidy wants auto obj here.

rwgk · 2024-08-11T06:16:19Z

include/pybind11/warnings.h

+inline void
+warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
+    if (!detail::PyWarning_Check(category.ptr())) {
+        pybind11_fail("raise_warning(): cannot raise warning, category must be a subclass of "


pybind11_fail("wanings::warn(): ...")

Same as #5291 (comment)

rwgk · 2024-08-16T14:47:32Z

include/pybind11/warnings.h

+                      + std::string(name) + "\" exists already.");
+    }
+    std::string full_name = scope.attr("__name__").cast<std::string>() + std::string(".") + name;
+    auto *new_ex = PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), nullptr);


Two separate things things:

Using handle here is very common around the pybind11 codebase, before any nullptr check. The if (!new_ex) below can stay as is.

Question: Is the const_cast still needed? Could you please try? — Recently we removed Python 3.7 support on master.

Changed back to handle.

The const_cast was originally not there, so it can be removed (as my local builds and now CIs shows to be true).

rwgk · 2024-08-16T14:59:21Z

tests/test_warnings.cpp

+
+#include <utility>
+
+namespace warning_helpers {


Could you please put this in a namespace?

Usually, in new code, I'd have this here:

namespace pybind11_tests { namespace warnings {

This is because we're linking most tests together into one extension (something I wanted to change for years, but it's still like that). Without using namespaces we might get accidental ODR violations (in the link step).

See #5291 (comment)

rwgk · 2024-08-16T15:12:26Z

tests/test_warnings.cpp

+          []() { py::warnings::warn("UnicodeWarning is raised!", PyExc_UnicodeWarning); });
+
+    m.def("raise_and_fail",
+          []() { py::warnings::warn("RuntimeError should be raised!", PyExc_Exception); });


This confused me:

"RuntimeError should be raised!"

Suggestion to change to:

"Invalid category"

I'd also change raise_and_fail to raise_invalid_category

Actually: is "raise" a good term to use here (all test functions)?

Because there is no "raise" in warnings.warn().

Maybe warn_with_invalid_category etc? So that it is more clear to someone who gets here from a different context (e.g. refactoring or cleanup) what's happening.

I see the motivation, I hope now names and messages are more self-explainatory/clear.

rwgk · 2024-08-16T15:19:08Z

tests/test_warnings.cpp

+    warning_helpers::warn_function(
+        m, "raise_bytes_warning", PyExc_BytesWarning, "This is BytesWarning!");
+    warning_helpers::warn_function(
+        m, "raise_deprecation_warning", PyExc_DeprecationWarning, "This is DeprecationWarning!");


I'd delete all warning below: mostly because it doesn't exercise anything specific in your implementation, but also because it will look old after a few years, because we're likely to not keep up with new warning types, and the wall of repetitive code is distracting from potentially more important things.

I think it's best to only keep PyExc_Warning (the base class) and one randomly picked derived class.

I have removed them all as other functions like warn_and_return_value or warn_with_different_category use these categories. This also result in obsolecence of namespace pybind11_tests/warnings as no helpers are needed now (ref #5291 (comment)).

…_feature

rwgk

Looks great! My only question is about using @pytest.mark.parametrize() where there is only actually one test.

I only have/had a few minutes right now. I'll try to find more time later for another more careful pass.

rwgk · 2024-08-20T15:27:30Z

tests/test_warnings.py

+        "module_function",
+    ),
+    [
+        (


This seems to be the only one list item.

I'd either remove the @pytest.mark.parametrize() here and just assign the variables directly (e.g. expected_value = 37) or try to think if there could be another test we could meaningfully add here.

Yeah, I guess it's a leftover from previous iterations (not sure but looks like one:)). Removed!

…_feature

rwgk

Looks great, thanks!

I'll wait a couple days to give others a chance to look, too: @henryiii, @EthanSteinberg, @Skylion007

EthanSteinberg

This looks good to me. Thanks for the contribution!

jiwaszki · 2024-08-29T07:41:28Z

@rwgk @EthanSteinberg thank you as well!

One more question, is it okay if I add documentation later around end of this/next week?

rwgk · 2024-08-29T08:40:40Z

Yes, that will be great.

I merged this PR already. Please open a new PR for the documentation.

Add warning wrappers that allow to call warnings from pybind level

0afa32f

jiwaszki requested a review from henryiii as a code owner August 8, 2024 23:41

rwgk reviewed Aug 11, 2024

View reviewed changes

jiwaszki added 3 commits August 15, 2024 02:18

Add missing include for warnings.h

3233262

Change messages on failed checks, extend testing

0dcd917

clang-tidy fix

1f50050

rwgk reviewed Aug 16, 2024

View reviewed changes

jiwaszki added 3 commits August 19, 2024 03:08

Refactor tests for warnings

76d9e10

Move handle before check

f4cf539

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

68cfd13

…_feature

rwgk reviewed Aug 20, 2024

View reviewed changes

jiwaszki added 2 commits August 26, 2024 00:34

Remove unnecessary parametrized

f98b5d4

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

24cebfa

…_feature

jiwaszki requested a review from rwgk August 27, 2024 16:06

rwgk approved these changes Aug 27, 2024

View reviewed changes

EthanSteinberg approved these changes Aug 29, 2024

View reviewed changes

rwgk merged commit 66c3774 into pybind:master Aug 29, 2024
78 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 29, 2024

jiwaszki mentioned this pull request Sep 5, 2024

Add documentation entry for warnings #5356

Open

henryiii changed the title ~~Warnings wrappers to use from C++~~ feat: warnings wrappers to use from C++ Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jiwaszki commented Aug 8, 2024 •

edited by henryiii

Loading

jiwaszki commented Aug 8, 2024

rwgk left a comment

rwgk Aug 11, 2024

jiwaszki Aug 15, 2024 •

edited

Loading

rwgk Aug 16, 2024

rwgk Aug 11, 2024

jiwaszki Aug 15, 2024

rwgk Aug 11, 2024

jiwaszki Aug 15, 2024

rwgk Aug 11, 2024

jiwaszki Aug 15, 2024

rwgk Aug 11, 2024

rwgk Aug 11, 2024

jiwaszki Aug 15, 2024

rwgk Aug 16, 2024

jiwaszki Aug 19, 2024

rwgk Aug 16, 2024 •

edited

Loading

jiwaszki Aug 19, 2024

rwgk Aug 16, 2024

jiwaszki Aug 19, 2024 •

edited

Loading

rwgk Aug 16, 2024

jiwaszki Aug 19, 2024

rwgk left a comment

rwgk Aug 20, 2024

jiwaszki Aug 25, 2024

rwgk left a comment

EthanSteinberg left a comment

jiwaszki commented Aug 29, 2024

rwgk commented Aug 29, 2024

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

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

Conversation

jiwaszki commented Aug 8, 2024 • edited by henryiii Loading

Description

Suggested changelog entry:

jiwaszki commented Aug 8, 2024

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiwaszki Aug 15, 2024 • edited Loading

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

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

rwgk Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiwaszki Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

EthanSteinberg left a comment

Choose a reason for hiding this comment

jiwaszki commented Aug 29, 2024

rwgk commented Aug 29, 2024

jiwaszki commented Aug 8, 2024 •

edited by henryiii

Loading

jiwaszki Aug 15, 2024 •

edited

Loading

rwgk Aug 16, 2024 •

edited

Loading

jiwaszki Aug 19, 2024 •

edited

Loading