Add helper for parsing tuples using nanobind #3025
Conversation
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
4e5864d
to
72584bc
Compare
|
✔️ 72584bc -> Azure artifacts URL |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3025 +/- ##
==========================================
- Coverage 67.27% 67.27% -0.01%
==========================================
Files 572 573 +1
Lines 104944 104944
==========================================
- Hits 70605 70597 -8
- Misses 34339 34347 +8 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
| return NULL; | ||
| } | ||
| bool b = hoc_valid_stmt(cmd, 0); | ||
| auto [cmd] = nrn::cast_tuple<const std::string>(nb::tuple(args)); |
There was a problem hiding this comment.
I have noticed that some of these constructors are the equivalent of the python constructor of the same name, which is also the case here. nb::tuple() calls either PySequence_Tuple or PyTuple_New effectively behaving as python's tuple()
I have manually steal or borrow and let the object be moved
(nb::tuple(nb::borrow(args)) in this case). Not sure if there is anything to be gained, it's mostly for info.
There was a problem hiding this comment.
I'm not sure I follow what you mean by:
I have manually steal or borrow and let the object be moved (nb::tuple(nb::borrow(args)) in this case)
ie: what you mean by moved in the above, and why would one add the extra nb::borrow()?
There was a problem hiding this comment.
My comment is basically, there's a lot going on in the background when you do nb::tuple(), whereas if you would have done nb::tuple(nb::borrow(args) there's an explicit creation of nb::object with a borrowed reference, which being an rvalue-ref it's moved to be a nb::tuple in an almost-no-op.
Since I don't fully know what your direct call implicitly does, sometimes being explicit is better. But if you find out it does the very right (and minimal) thing I'd be glad to know.
There was a problem hiding this comment.
Sorry, I was on vacation and missed this.
whereas if you would have done nb::tuple(nb::borrow(args) there's an explicit creation of nb::object with a borrowed reference
I'm all for being explicit, but I'm not sure why I want to nb::borrow; the refcnt doesn't need to be incremented from what I can tell, why would we need to do that?
There was a problem hiding this comment.
nb::tuple extends nb::object, so there will be decref at the end of its life. I believe in this case we must borrow, not steal. Hence my comment, maybe we would better be explicit about it.
There was a problem hiding this comment.
nb::tuple extends nb::object, so there will be decref at the end of its life.
I don't see why that is a problem. The nb::tuple calls https://github.com/wjakob/nanobind/blob/master/include/nanobind/nb_types.h#L478 which then calls https://github.com/wjakob/nanobind/blob/master/src/common.cpp#L623
This is then used to initialize the nb::object, so it has the correct refcount, and thus when it is dtor'd, everything is correct.
Note that both code paths go through PySequence_Tuple, so a new python tuple is created with its own lifetime and refcount.
src/nrnpython/utils.hpp
Outdated
| if (I >= t.size()) { | ||
| throw std::runtime_error("Not enough arguments"); |
There was a problem hiding this comment.
I think we should let t[I] do its Python thing. Upon out-of-bounds it will set the Python exception and return Null, which is likely what we want
https://docs.python.org/3/c-api/tuple.html#c.PyTuple_GetItem
There was a problem hiding this comment.
I think it's better if we are helpful with our error messages. I think that my current exception isn't worded well, and could be better, but that's a detail I need to fill in later.
More importantly, without the check, the code will segfault. Perhaps you have a different implementation in mind?
There was a problem hiding this comment.
So I guess something is confusing me. Why would t[i] segfault if it only calls PyTuple_GetItem under the hood?
There was a problem hiding this comment.
We now also have access to libfmt, so you can use that to improve the error message.
There was a problem hiding this comment.
I have updated the error message in the newest version.
src/nrnpython/utils.hpp
Outdated
| @@ -0,0 +1,31 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
The file can be called nrnpython/cast_tuple.hpp.
src/nrnpython/utils.hpp
Outdated
| } // namespace detail | ||
|
|
||
| template <typename... Ts, typename Tuple> | ||
| auto cast_tuple(Tuple&& tuple) { |
There was a problem hiding this comment.
Why any Tuple, if we know it's a const nanobind::tuple&?
There was a problem hiding this comment.
I was running into an issue trying to handle the nb::args case, and that's how I ended up working around it.
I have added a test if you want to try/suggest alternatives.
This comment has been minimized.
This comment has been minimized.
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
2bfe2e2
to
8ab8a6c
Compare
* This can also be used for parsing `args`
mgeplf
force-pushed
the
nanobind-parse-tuple
branch
from
8ab8a6c
to
63e575a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
✔️ 63e575a -> Azure artifacts URL |
|
|
✔️ 738ff1c -> Azure artifacts URL |
|
@alkino / @1uc I seem to be getting errors with nanobind not being available: I'm confused as to why the tests are "automatically" including nanobind: My plan to deal with it is to only include the |
args