[mlir][py] Mark all type caster from_{cpp,python} methods as noexcept
#143866
Conversation
This is mentioned as a "must" in https://nanobind.readthedocs.io/en/latest/porting.html#type-casters when implementing type casters. While most of the existing `from_cpp` methods were already marked noexcept, many of the `from_python` methods were not. This commit adds the missing noexcept declarations to all type casters found in `NanobindAdaptors.h`.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir Author: Nicholas Junge (nicholasjng) ChangesThis is mentioned as a "must" in https://nanobind.readthedocs.io/en/latest/porting.html#type-casters when implementing type casters. While most of the existing Full diff: https://github.com/llvm/llvm-project/pull/143866.diff 1 Files Affected:
diff --git a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
index 2dd35c097c796..e39b1a752f8d6 100644
--- a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
+++ b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
@@ -67,7 +67,7 @@ static nanobind::object mlirApiObjectToCapsule(nanobind::handle apiObject) {
template <>
struct type_caster<MlirAffineMap> {
NB_TYPE_CASTER(MlirAffineMap, const_name("MlirAffineMap"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToAffineMap(capsule.ptr());
if (mlirAffineMapIsNull(value)) {
@@ -90,7 +90,7 @@ struct type_caster<MlirAffineMap> {
template <>
struct type_caster<MlirAttribute> {
NB_TYPE_CASTER(MlirAttribute, const_name("MlirAttribute"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToAttribute(capsule.ptr());
return !mlirAttributeIsNull(value);
@@ -111,7 +111,7 @@ struct type_caster<MlirAttribute> {
template <>
struct type_caster<MlirBlock> {
NB_TYPE_CASTER(MlirBlock, const_name("MlirBlock"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToBlock(capsule.ptr());
return !mlirBlockIsNull(value);
@@ -122,7 +122,7 @@ struct type_caster<MlirBlock> {
template <>
struct type_caster<MlirContext> {
NB_TYPE_CASTER(MlirContext, const_name("MlirContext"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
if (src.is_none()) {
// Gets the current thread-bound context.
// TODO: This raises an error of "No current context" currently.
@@ -142,7 +142,7 @@ struct type_caster<MlirContext> {
template <>
struct type_caster<MlirDialectRegistry> {
NB_TYPE_CASTER(MlirDialectRegistry, const_name("MlirDialectRegistry"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToDialectRegistry(capsule.ptr());
return !mlirDialectRegistryIsNull(value);
@@ -162,7 +162,7 @@ struct type_caster<MlirDialectRegistry> {
template <>
struct type_caster<MlirLocation> {
NB_TYPE_CASTER(MlirLocation, const_name("MlirLocation"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
if (src.is_none()) {
// Gets the current thread-bound context.
src = nanobind::module_::import_(MAKE_MLIR_PYTHON_QUALNAME("ir"))
@@ -188,7 +188,7 @@ struct type_caster<MlirLocation> {
template <>
struct type_caster<MlirModule> {
NB_TYPE_CASTER(MlirModule, const_name("MlirModule"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToModule(capsule.ptr());
return !mlirModuleIsNull(value);
@@ -209,12 +209,13 @@ template <>
struct type_caster<MlirFrozenRewritePatternSet> {
NB_TYPE_CASTER(MlirFrozenRewritePatternSet,
const_name("MlirFrozenRewritePatternSet"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToFrozenRewritePatternSet(capsule.ptr());
return value.ptr != nullptr;
}
- static handle from_cpp(MlirFrozenRewritePatternSet v, rv_policy, handle) {
+ static handle from_cpp(MlirFrozenRewritePatternSet v, rv_policy,
+ handle) noexcept {
nanobind::object capsule = nanobind::steal<nanobind::object>(
mlirPythonFrozenRewritePatternSetToCapsule(v));
return nanobind::module_::import_(MAKE_MLIR_PYTHON_QUALNAME("rewrite"))
@@ -228,7 +229,7 @@ struct type_caster<MlirFrozenRewritePatternSet> {
template <>
struct type_caster<MlirOperation> {
NB_TYPE_CASTER(MlirOperation, const_name("MlirOperation"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToOperation(capsule.ptr());
return !mlirOperationIsNull(value);
@@ -250,7 +251,7 @@ struct type_caster<MlirOperation> {
template <>
struct type_caster<MlirValue> {
NB_TYPE_CASTER(MlirValue, const_name("MlirValue"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToValue(capsule.ptr());
return !mlirValueIsNull(value);
@@ -273,7 +274,7 @@ struct type_caster<MlirValue> {
template <>
struct type_caster<MlirPassManager> {
NB_TYPE_CASTER(MlirPassManager, const_name("MlirPassManager"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToPassManager(capsule.ptr());
return !mlirPassManagerIsNull(value);
@@ -284,7 +285,7 @@ struct type_caster<MlirPassManager> {
template <>
struct type_caster<MlirTypeID> {
NB_TYPE_CASTER(MlirTypeID, const_name("MlirTypeID"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToTypeID(capsule.ptr());
return !mlirTypeIDIsNull(value);
@@ -306,7 +307,7 @@ struct type_caster<MlirTypeID> {
template <>
struct type_caster<MlirType> {
NB_TYPE_CASTER(MlirType, const_name("MlirType"))
- bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) {
+ bool from_python(handle src, uint8_t flags, cleanup_list *cleanup) noexcept {
nanobind::object capsule = mlirApiObjectToCapsule(src);
value = mlirPythonCapsuleToType(capsule.ptr());
return !mlirTypeIsNull(value);
|
|
Please check if the test failure is relevant. I see |
|
Thanks for the review. It is relevant, since we're now terminating because of an exception thrown in one of the type casters. The llvm-project/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h Lines 53 to 57 in 349f8d6 I'm unsure how to proceed - on the one hand, the docs are pretty clear that cc @hawkinsp for a review (hope that's okay). |
|
It doesn't look like you will get a review from hawkinsp, they are not an active contributor to MLIR.
If you have a particular refactoring in mind, feel free to propose it. |
|
Please do ping me if a review is needed. |
|
Fair. I thought since he authored that code, I might get some insights about what I have to watch out for. I propose to make The error handling on the Python side should stay about the same, since nanobind itself will throw a type error after it exhaused all type caster overloads for a given type. But I should implement it first before making assumptions. I'll ping you when I'm ready with the refactoring. Thanks for coming back to this! |
|
Hey, I'm still on this. My current idea is to return an invalid handle from Otherwise, my idea is to just return I'll have to read up on the Python lit testing as well, since I don't know how to interpret the failed test output correctly (see below). But I'm working on it. Test output |
|
I would say just use |
|
I can try it and evaluate the change in the tests. |
Instead of raising a `nanobind::type_error()`. This is necessary to honor the nanobind type caster API contract, which requires `from_python` and `from_cpp` methods to be marked `noexcept`.
| value = mlirPythonCapsuleToAffineMap(capsule.ptr()); | ||
| if (mlirAffineMapIsNull(value)) { | ||
| std::optional<nanobind::object> capsule = mlirApiObjectToCapsule(src); | ||
| if (!capsule) |
There was a problem hiding this comment.
I did
if (auto capsule = mlirApiObjectToCapsule(src))
value = mlirPythonCapsuleToPassManager(capsule->ptr());
|
Here's a working/passing diff https://gist.github.com/makslevental/b224ffca7f15e273a4897975cda28b4c |
|
Thanks, that's pretty cool. Although now I'm wondering why I didn't achieve the same with my previous Would you like to commit your version instead, then? Or how should we proceed? |
Just apply the patch your PR here - it's fine (I don't care "about" credit, just that it gets landed 😄) |
|
Thank you! I'm happy to credit you in any way you choose, should you change your mind. The patch gives me a segmentation violation on my local machine (M1 Mac), though: Also, the test still does not pass. Is something wrong with my rebuilding workflow? I just run |
weird because i wrote and tested the patch on my m1... you're gonna have to build in debug mode to figure out where this is coming from |
|
FYI I appreciate the shoutout in the message but can you actually remove my handle from the commit message (otherwise I get pings for weeks when people rebase/merge in the change 😅). |
|
Absolutely, give me a second. |
Modeled after a patch posted in the conversation in PR llvm#143866: https://gist.github.com/makslevental/b224ffca7f15e273a4897975cda28b4c.
nicholasjng
force-pushed
the
constexpr-from-cpp
branch
from
d0c4d2f to
db8e35c
Compare
|
Lol okay it failed in CI - gotta whip out a linux machine to test. |
|
Interesting. It started passing locally for me when I rebuilt in debug mode. I guess it was a caching issue on my end (I set ccache to be on for the build). |
|
It's coming from here https://github.com/wjakob/nanobind/blob/master/include/nanobind/nb_cast.h#L576 which is a It's not actually a crash right - it's |
|
It's funny - what's happening is the cast is failing here MlirAttribute rawAttribute =
nanobind::cast<MlirAttribute>(otherAttribute);
if (!isaFunction(rawAttribute)) {
auto origRepr =
nanobind::cast<std::string>(nanobind::repr(otherAttribute));
throw std::invalid_argument(
(llvm::Twine("Cannot cast attribute to ") + captureTypeName +
" (from " + origRepr + ")")
.str());
}specifically After stepping through a bunch I believe we're facing the fact that since |
|
Yea this bears further investigation. I can't get to this until late next week so @nicholasjng feel free to keep pushing (but don't worry if you can't figure it out). |
|
I guess the solution, if we do this, is to replace all the casts with |
This avoids a nb::cast_error throw in certain class conversions.
|
Thanks! I started checking the casts, and found two more llvm-project/mlir/lib/Bindings/Python/IRModule.h Lines 1010 to 1012 in 5fd319f llvm-project/mlir/lib/Bindings/Python/IRModule.h Lines 1143 to 1145 in 5fd319f However, they are casting from an It might also be necessary to check some of the other |
Yea I saw these and I think they're safe according to exact your same logic.
Yup those are exactly the ones I was worried about. Note though (I believe!) any of the casts like Also btw we're gonna need a PSA about this because it's a breaking change (even though it's a fix) for downstream projects. |
|
Sure. Is there a process documentation for PSAs? Since there are no other observable breakages with the current available tests, would you consider this otherwise complete for now? |
I just mean you should make a post on the discourse like https://discourse.llvm.org/t/psa-opty-create-now-with-100-more-tab-complete/87339
Yup - should I merge it for you? |
|
That would be nice. I'll craft a PSA sometime later today. Thanks for all the help! |
|
@nicholasjng Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
After llvm#143866, we no longer always write to `value`, causing it to be uninitialized. This can lead to mysterious crashes, e.g. in `testCustomAttribute` when we attempt to evaluate `TestAttr(42)`, it does not set `value`, but `mlirAttributeIsNull(value)` happens to return false for garbage memory, and we end up trying to interpret it as a function instead of skipping it.
|
FYI, this commit causes some crashes in python_test.py, at least when running downstream -- haven't checked buildbots. Created #148944 to address the msan issues. |
After #143866, we no longer always write to `value`, causing it to be uninitialized. This can lead to mysterious crashes, e.g. in `python_test.py` / `testCustomAttribute` when we attempt to evaluate `TestAttr(42)`, it does not set `value`, but `mlirAttributeIsNull(value)` happens to return false for garbage memory, and we end up trying to interpret it as a function instead of skipping it. Fix this by only reading `value` if it has been assigned. If it hasn't, `return false` seems the right choice for all these methods, i.e. indicate that `from_python` failed.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/38192 Here is the relevant piece of the build log for the reference |
This is mentioned as a "must" in https://nanobind.readthedocs.io/en/latest/porting.html#type-casters when implementing type casters.
While most of the existing
from_cppmethods were already marked noexcept, many of thefrom_pythonmethods were not. This commit adds the missing noexcept declarations to all type casters found inNanobindAdaptors.h.