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.

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

Handling warnings from C++ #4535

Closed
wants to merge 9 commits into from

Conversation

jiwaszki
Copy link
Contributor

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.

Copy link
Collaborator

@EthanSteinberg EthanSteinberg left a comment

Choose a reason for hiding this comment

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

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

if (result == 1) {
return true;
}
if (result == -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

PYBIND11_NAMESPACE_BEGIN(warnings)

// Warning class
static PyObject *warning_base = PyExc_Warning;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why aren't these const?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even constexpr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

import warnings
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@EthanSteinberg EthanSteinberg left a comment

Choose a reason for hiding this comment

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

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/

"PyExc_Warning!");
}

if (PyErr_WarnEx(const_cast<PyObject *>(category), message, stack_level) == -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These const casts aren't safe

@jiwaszki
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return true;
}
if (result == -1) {
PyErr_Clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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().

[]() { py::raise_warning("RuntimeError should be raised!", PyExc_Exception); });

// Test custom warnings
static py::warning<CustomWarning> my_warning(m, "CustomWarning", py::warnings::deprecation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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_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"); });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants