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

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

Merged
merged 9 commits into from
Aug 29, 2024
3 changes: 2 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ set(PYBIND11_HEADERS
include/pybind11/stl_bind.h
include/pybind11/stl/filesystem.h
include/pybind11/type_caster_pyobject_ptr.h
include/pybind11/typing.h)
include/pybind11/typing.h
include/pybind11/warnings.h)

# Compare with grep and warn if mismatched
if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)
Expand Down
76 changes: 76 additions & 0 deletions include/pybind11/warnings.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
pybind11/warnings.h: Python warnings wrappers.

Copyright (c) 2024 Jan Iwaszkiewicz <[email protected]>

All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/

#pragma once

#include "pybind11.h"
#include "detail/common.h"

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

PYBIND11_NAMESPACE_BEGIN(detail)

inline bool PyWarning_Check(PyObject *obj) {
int result = PyObject_IsSubclass(obj, PyExc_Warning);
if (result == 1) {
return true;
}
if (result == -1) {
raise_from(PyExc_SystemError,
"pybind11::detail::PyWarning_Check(): PyObject_IsSubclass() call failed.");
throw error_already_set();
}
return false;
}

PYBIND11_NAMESPACE_END(detail)

PYBIND11_NAMESPACE_BEGIN(warnings)

inline object
new_warning_type(handle scope, const char *name, handle base = PyExc_RuntimeWarning) {
if (!detail::PyWarning_Check(base.ptr())) {
pybind11_fail("pybind11::warnings::new_warning_type(): cannot create custom warning, base "
"must be a subclass of "
"PyExc_Warning!");
}
if (hasattr(scope, name)) {
pybind11_fail("pybind11::warnings::new_warning_type(): an attribute with name \""
+ 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (!new_ex) {
raise_from(PyExc_SystemError,
"pybind11::warnings::new_warning_type(): PyErr_NewException() call failed.");
throw error_already_set();
}
handle h(new_ex);
auto obj = reinterpret_steal<object>(h);
scope.attr(name) = obj;
return obj;
}

// Similar to Python `warnings.warn()`
inline void
warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
if (!detail::PyWarning_Check(category.ptr())) {
pybind11_fail(
"pybind11::warnings::warn(): cannot raise warning, category must be a subclass of "
"PyExc_Warning!");
}

if (PyErr_WarnEx(category.ptr(), message, stack_level) == -1) {
throw error_already_set();
}
}

PYBIND11_NAMESPACE_END(warnings)

PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
3 changes: 2 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ set(PYBIND11_TEST_FILES
test_unnamed_namespace_a
test_unnamed_namespace_b
test_vector_unique_ptr_member
test_virtual_functions)
test_virtual_functions
test_warnings)

# Invoking cmake with something like:
# cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" ..
Expand Down
1 change: 1 addition & 0 deletions tests/extra_python_package/test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"include/pybind11/stl_bind.h",
"include/pybind11/type_caster_pyobject_ptr.h",
"include/pybind11/typing.h",
"include/pybind11/warnings.h",
}

detail_headers = {
Expand Down
79 changes: 79 additions & 0 deletions tests/test_warnings.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
tests/test_warnings.cpp -- usage of warnings::warn() and warnings categories.

Copyright (c) 2024 Jan Iwaszkiewicz <[email protected]>

All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/

#include <pybind11/warnings.h>

#include "pybind11_tests.h"

#include <utility>

namespace warning_helpers {
Copy link
Collaborator

@rwgk rwgk Aug 16, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

void warn_function(py::module &m, const char *name, py::handle category, const char *message) {
m.def(name, [category, message]() { py::warnings::warn(message, category); });
}
} // namespace warning_helpers

class CustomWarning {};

TEST_SUBMODULE(warnings_, m) {

// Test warning mechanism base
m.def("raise_and_return", []() {
std::string message = "Warning was raised!";
py::warnings::warn(message.c_str(), PyExc_Warning);
return 21;
});

m.def("raise_default", []() { py::warnings::warn("RuntimeWarning is raised!"); });

m.def("raise_from_cpython",
[]() { py::warnings::warn("UnicodeWarning is raised!", PyExc_UnicodeWarning); });

m.def("raise_and_fail",
[]() { py::warnings::warn("RuntimeError should be raised!", PyExc_Exception); });
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jiwaszki jiwaszki Aug 19, 2024

Choose a reason for hiding this comment

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

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


// Test custom warnings
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
ex_storage.call_once_and_store_result([&]() {
return py::warnings::new_warning_type(m, "CustomWarning", PyExc_DeprecationWarning);
});

m.def("raise_custom", []() {
py::warnings::warn("CustomWarning was raised!", ex_storage.get_stored());
return 37;
});

m.def("register_duplicate_warning",
[m]() { py::warnings::new_warning_type(m, "CustomWarning", PyExc_RuntimeWarning); });

// Bind warning categories
warning_helpers::warn_function(m, "raise_base_warning", PyExc_Warning, "This is Warning!");
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!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

warning_helpers::warn_function(
m, "raise_future_warning", PyExc_FutureWarning, "This is FutureWarning!");
warning_helpers::warn_function(
m, "raise_import_warning", PyExc_ImportWarning, "This is ImportWarning!");
warning_helpers::warn_function(m,
"raise_pending_deprecation_warning",
PyExc_PendingDeprecationWarning,
"This is PendingDeprecationWarning!");
warning_helpers::warn_function(
m, "raise_resource_warning", PyExc_ResourceWarning, "This is ResourceWarning!");
warning_helpers::warn_function(
m, "raise_runtime_warning", PyExc_RuntimeWarning, "This is RuntimeWarning!");
warning_helpers::warn_function(
m, "raise_syntax_warning", PyExc_SyntaxWarning, "This is SyntaxWarning!");
warning_helpers::warn_function(
m, "raise_unicode_warning", PyExc_UnicodeWarning, "This is UnicodeWarning!");
warning_helpers::warn_function(
m, "raise_user_warning", PyExc_UserWarning, "This is UserWarning!");
}
115 changes: 115 additions & 0 deletions tests/test_warnings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
from __future__ import annotations

import warnings

import pytest

import pybind11_tests # noqa: F401
from pybind11_tests import warnings_ as m


@pytest.mark.parametrize(
("expected_category", "expected_message", "expected_value", "module_function"),
[
(Warning, "Warning was raised!", 21, m.raise_and_return),
(RuntimeWarning, "RuntimeWarning is raised!", None, m.raise_default),
(UnicodeWarning, "UnicodeWarning is raised!", None, m.raise_from_cpython),
],
)
def test_warning_simple(
expected_category, expected_message, expected_value, module_function
):
with pytest.warns(Warning) as excinfo:
value = module_function()

assert issubclass(excinfo[0].category, expected_category)
assert str(excinfo[0].message) == expected_message
assert value == expected_value


def test_warning_wrong_subclass_fail():
with pytest.raises(Exception) as excinfo:
m.raise_and_fail()

assert issubclass(excinfo.type, RuntimeError)
assert (
str(excinfo.value)
== "pybind11::warnings::warn(): cannot raise warning, category must be a subclass of PyExc_Warning!"
)


def test_warning_double_register_fail():
with pytest.raises(Exception) as excinfo:
m.register_duplicate_warning()

assert issubclass(excinfo.type, RuntimeError)
assert (
str(excinfo.value)
== 'pybind11::warnings::new_warning_type(): an attribute with name "CustomWarning" exists already.'
)


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

with pytest.warns(m.CustomWarning) as excinfo:
warnings.warn("This is warning from Python!", m.CustomWarning, stacklevel=1)

assert issubclass(excinfo[0].category, DeprecationWarning)
assert issubclass(excinfo[0].category, m.CustomWarning)
assert str(excinfo[0].message) == "This is warning from Python!"


@pytest.mark.parametrize(
(
"expected_category",
"expected_base",
"expected_message",
"expected_value",
"module_function",
),
[
(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

m.CustomWarning,
DeprecationWarning,
"CustomWarning was raised!",
37,
m.raise_custom,
),
],
)
def test_warning_custom(
expected_category, expected_base, expected_message, expected_value, module_function
):
with pytest.warns(expected_category) as excinfo:
value = module_function()

assert issubclass(excinfo[0].category, expected_base)
assert issubclass(excinfo[0].category, expected_category)
assert str(excinfo[0].message) == expected_message
assert value == expected_value


@pytest.mark.parametrize(
("expected_category", "module_function"),
[
(Warning, m.raise_base_warning),
(BytesWarning, m.raise_bytes_warning),
(DeprecationWarning, m.raise_deprecation_warning),
(FutureWarning, m.raise_future_warning),
(ImportWarning, m.raise_import_warning),
(PendingDeprecationWarning, m.raise_pending_deprecation_warning),
(ResourceWarning, m.raise_resource_warning),
(RuntimeWarning, m.raise_runtime_warning),
(SyntaxWarning, m.raise_syntax_warning),
(UnicodeWarning, m.raise_unicode_warning),
(UserWarning, m.raise_user_warning),
],
)
def test_warning_categories(expected_category, module_function):
with pytest.warns(Warning) as excinfo:
module_function()

assert issubclass(excinfo[0].category, expected_category)
assert str(excinfo[0].message) == f"This is {expected_category.__name__}!"