Conversation
|
The wheel builds for free threaded python is fixed with these changes https://github.com/scipp/scipp/actions/runs/22350551565 |
jl-wynen
left a comment
There was a problem hiding this comment.
I don't really understand the C API and the documentation is lacking. But I don't see how this PR could be harmful and since it seems to solve the issue, I think we can merge this after some small improvements.
lib/python/bind_operators.h
Outdated
| [](py::object &a, Other &b) -> py::object & { | ||
| a.cast<T &>() += RHSSetup{}(b); | ||
| auto &self = a.cast<T &>(); | ||
| auto &&rhs = RHSSetup{}(b); |
There was a problem hiding this comment.
Does RHSSetup need the thread state?
There was a problem hiding this comment.
Nope, it's in cpp land. Updated and pushed again.
lib/python/bind_operators.h
Outdated
| // thread state, and a.cast<T &>() may trigger PyCriticalSection_Begin | ||
| // which requires the thread state, causing a segfault. Instead, we | ||
| // extract the C++ reference first, then release the GIL only around the | ||
| // pure C++ operation. |
There was a problem hiding this comment.
You lost the original comment. I think explaining why we return by reference here is still relevant.
There was a problem hiding this comment.
There is now a lot of repeated, non-trivial code here. Can you extract that into a reusable function? E.g., using a callback like
c.def(
"__iadd__",
inplace_operator([](auto &&lhs, auto &&rhs) { lhs += rhs; }),
py::is_operator());
lib/python/bind_operators.h
Outdated
| }; | ||
| inplace_logical("__ior__", [](auto &a, const auto &b) { a |= b; }); | ||
| inplace_logical("__ixor__", [](auto &a, const auto &b) { a ^= b; }); | ||
| inplace_logical("__iand__", [](auto &a, const auto &b) { a &= b; }); |
There was a problem hiding this comment.
Can you use the same pattern for logical and arithmetic operators? That is, either use a function like inplace_logical for all or the c.def("...", inplace_op...) pattern for all.
Also, how come arithmetic uses (auto &a, auto &&b) and logical uses (auto &a, const auto &b)?
Cherry picks c95645f from #3852
The wheel builds for cp14t were failing https://github.com/scipp/scipp/actions/runs/22346477173
This is Claude's reasoning while trying to push it: