-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Extend HPy call API #251
Extend HPy call API #251
Conversation
The previous discussion of some of the options: #122 |
🤦 Thanks, @hodgestar, completely overlooked this issue. |
9fa9f02
to
334ed17
Compare
@hodgestar @rlamy @antocuni @mattip This should be ready for a second review round (actually, the first serious one). |
@fangerer For example,
If we plan to implement the vectorcall protocol, we should probably add it to objects too and match the semantics and name the methods to match (e.g. If we don't plan to implement the vectorcall protocol, we should probably not use Hopefully this doesn't mess with your plans too much -- I discovered all of this while writing up #122 ages ago, but I don't think it made it into the issue write up very clearly. :/ |
@hodgestar Thanks for your comments.
Sure, but it's the API with the lowest overhead if
I'm aware of that and I'm sure that we agreed on not not doing the same.
It's still fast if no keyword arguments were specified because it then doesn't need to unpack anything. Sorry, I forgot to add some comments about this. According to the analysis of the top4000 packages (https://paste.opendev.org/show/bPLIEafAmoHq5prb9F8f/), the most frequently used call functions are
Good question. I'm not sure if it makes sense to implement the vectorcall protocol. CPython also uses it internally but PyPy and Graalpython don't. Furthermore, our
Don't worry, I just try to make some progress. There is no need to rush such things. |
hpy/debug/src/debug_ctx.c
Outdated
for(int i=0; i<nargs; i++) { | ||
uh_args[i] = DHPy_unwrap(dctx, dh_args[i]); | ||
} | ||
return DHPy_open(dctx, HPy_CallMethodVectorDict(get_info(dctx)->uctx, DHPy_unwrap(dctx, dh_receiver), DHPy_unwrap(dctx, dh_name), uh_args, nargs, DHPy_unwrap(dctx, dh_kw))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: we don't have a official style guidelines (maybe we should?) but I think that 178 chars is definitely too long for a single line :)
hpy/devel/src/runtime/ctx_call.c
Outdated
|
||
#if PY_VERSION_HEX >= 0x03080000 | ||
PyObject **py_args = (PyObject **) alloca(nargs * sizeof(PyObject *)); | ||
_harr2pyarr(py_args, 0, args, nargs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment above. On the CPython ABI, this should be a noop cast
hpy/devel/src/runtime/ctx_call.c
Outdated
py_args = (PyObject **) alloca((nargs + 1) * sizeof(PyObject *)); | ||
py_args[0] = _h2py(receiver); | ||
_harr2pyarr(py_args, 1, args, nargs); | ||
result = PyObject_VectorcallMethod(_h2py(name), py_args, nargs + 1, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above about _harr2pyarr
on CPython ABI, but this is trickier (impossible?) because of the first argument.
PEP 590/VectorCall tries to solve a similar issue by using PY_VECTORCALL_ARGUMENTS_OFFSET
which allows to mutate the first argument of the vector.
However, I think that the only way to take advantage of this would be to refactor the C signature of HPy_CallMethodVectorDict
to pass receiver
as the args[0]
. But the for consistency we should probably change also the signatue of our HPyFunc_KEYWORDS
methods... what a mess 🤦♂️ .
) | ||
for receiver, args_tuple, kw in test_args: | ||
assert mod.call(receiver, *args_tuple, kw=kw) == receiver(*args_tuple, **kw) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's just me, but I find this style of testing very difficult to read. To start with the comment # (receiver, method, args_tuple)
looks wrong.
I think the following is orders or magnitude easier to read.
assert mod.call(dict, dict(a=0, b=1), kw={}) == dict((dict(a=0, b=1)) # or even == {'a': 0, 'b': 1}
assert mod.call(dict, kw=dict(a=0, b=1)) == dict(a=0, b=1)
If the code above is not equivalent to the original code, it's a good sign that the original code is too complex to read 😅.
Feel free to ignore this suggestion though.
I left some comments inline in the review. However, this probably requires a much deeper discussion. |
Hi all -- just a quick bit of feedback after having been pointed to this PR by @mattip. I looks like HPy at this point provides most of the facilities that would be needed to create an extra backend for nanobind (A hobby project of mine for C++ <-> Python interop, it's basically a simpler & faster version of pybind11), which would be very cool. However: nanobind derives a quite significant part of its speedup through the ability to receive and perform vector calls. As mentioned by @hodgestar above, the API proposed here involves quite a bit of boxing/unboxing and doesn't expose the fast way of performing calls. In response to @fangerer 's comment:
My answer would be a resounding YES!! 😊 |
@wjakob thanks for you comment and I agree. Since this PR is already a bit older, I'm not sure if I remember all the details correctly. The vectorcall protocol as such makes, for sure, sense in CPython. My question, if it makes sense, was because in HPy we would have the possibility to define a different protocol. But then we again need to do (costly) conversions in some cases which we should avoid.
I, of course, know nanobind and would love to see an HPy backend for it. I will revisit this PR in the near future but this PR alone won't be enough (since it's just about an object calling API like https://docs.python.org/3/c-api/call.html#object-calling-api). We will also need to have the ability to define types that implement the vectorcall protocol (i.e. specify |
Great, I am happy to hear it.
While object call protocol is helpful for callbacks and, e.g., to extend C++ classes in Python and override virtual functions there, those are IMO less common use cases. Binding code mostly involves C or C++ code called from Python, so it's the |
This has conflicts |
Big update on this PR: After PR #390 was merged, it now makes sense to introduce a call API that aligns with the signature of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
d2a4c10
to
dfd8b04
Compare
As discussed in #122 in this month's dev call, this PR adds APIs for calling functions and methods.
We don't want to add all the specific API functions like in the C API but we want to provide functions for two general use cases:
Hence, this PR adds following:
HPy_CallMethodTupleDict
/HPy_CallMethodTupleDict_s
:Call a method using an argument tuple and a keyword dict.
The name is given as Python unicode object (first variant) or as C string (suffix
_s
).HPy_CallVectorDict
:Call a callable object using a handle array and a keyword dict.
HPy_CallMethodVectorDict
:Call a method using a handle array and a keyword dict.
I'm still not sure about the best way how to specify the keywords.
CPython 3.9 basically provides both variants: providing them as Python dict or providing an array of C strings (and the values are appended to the arguments array).
For now, I've implemented the dict variant because I think it's not very common to craft and pass keyword args in C.
According to an analysis of the top4000, the most frequently used functions are
PyObject_CallMethod
andPyObject_CallFunction
and none of those allows to pass keyword arguments.