gh-98586: Add vector call APIs to the Limited API#98587

wjakob

This PR constains tentative changes needed to expose the facilities for making PEP-590-style vector calls through Python's limited API.

Issue: Add vector call functions to the limited API #98586

erlend-aasland

@markshannon might be interested in reviewing this as the author of PEP 590.

erlend-aasland

Could you add tests to Modules/_testcapimodule/vectorcall_limited.c and also adjust the NEWS entry to use Sphinx directives for linking to the relevant parts of the C-API docs and PEP-590?

wjakob

@erlend-aasland Done.

erlend-aasland

What is the reason for not adding PyVectorcall_Function to the Limited API?

wjakob

What is the reason for not adding PyVectorcall_Function to the Limited API?

It seemed like sort of an implementation detail. I am happy to add it (& tests) if there is consensus to do so.

erlend-aasland

It seemed like sort of an implementation detail. I am happy to add it (& tests) if there is consensus to do so.

I've never had use for it myself, so I have no opinion about this; I was just curious. Let's hear what the others say when they chime in.

markshannon

Two questions.

First question

Why include PyObject_VectorcallDict? It seem to be an internal function for efficiently supporting the old calling convention for callables that support vectorcall.

Second question

The vectorcall API has two parts.

Calling through PyObject_Vectorcall
Declaring that an object can be called as "vector callable" by setting the Py_TPFLAGS_HAVE_VECTORCALL flag and type.tp_vectorcall_offset.

Any plans to add the ability to declare classes as having "vector callable" instances?

wjakob

Thanks for the review @markshannon.

Why include PyObject_VectorcallDict? It seem to be an internal function for efficiently supporting the old calling convention for callables that support vectorcall.

I am happy to hear it. I have never found use for this function myself. I included in the PR for reasons of symmetry, and because it was prominently advertised in the docs next to PyObject_Vectorcall and PyObject_VectorcallMethod. I will remove it in the next update of the PR.

Any plans to add the ability to declare classes as having "vector callable" instances?

I think that this was already done in PR #93274. Please let me know if I misunderstood.

Finally, do you have any thoughts on PyVectorcall_Function brought up by @erlend-aasland ?

markshannon

One other question.

Why add PyObject_VectorcallMethod, rather than exposing the lower level method-getting machinery?
PyObject_VectorcallMethod forces the attribute lookup to happen after the arguments are evaluated which is different from Python.
A PyObject *LoadMethod(PyObject *obj, PyObject *name, PyObject **self) function would allow C code to use Python evaluation order. PyObject_VectorcallMethod could be added as a utility function if desirable.

This is a sketch and is probably missing some error handling and/or incref/decrefs.

PyObject *PyObject_VectorcallMethod(
    PyObject *name, PyObject *const *args,
    size_t nargsf, PyObject *kwnames)
{
      /* Error if nargs < 1 */
      PyObject *obj = args[0];
      PyObject *callable = LoadMethod(obj, name, &args[0]);
      /* TO DO --- Handle error if callable == NULL */
      if (args[0] == NULL) {
            /* No self */
           return PyObject_Vectorcall(callable, args+1, (nargsf-1) | PY_VECTORCALL_ARGUMENTS_OFFSET, kwnames);
      }
      return PyObject_Vectorcall(callable, args, nargsf, kwnames);
}

markshannon

Finally, do you have any thoughts on PyVectorcall_Function brought up by @erlend-aasland ?

I'd rather not add it.
Unless it is specified in such a way as to be useless, its existence would unnecessarily constrain the implementation.

wjakob

Why add PyObject_VectorcallMethod, rather than exposing the lower level method-getting machinery?
PyObject_VectorcallMethod forces the attribute lookup to happen after the arguments are evaluated which is different from Python.

It is convenient to use a 1-function-does-it-all interface like PyObject_VectorcallMethod for performing method calls from binding projects. This is particularly true for the Py_LIMITED_API that is already heavy on CPython API calls due to the opaque nature of the interfaces. The difference in attribute lookup ordering is an interesting point. I was not aware of this, but it strikes me as a quite minor/subtle issue. (Maybe others will disagree though!)

Altogether, it sounds to me more like a general question about why PyObject_VectorcallMethod was designed the way it was? My goal with this PR was to expose something that is already in general use.

wjakob

Is this okay @markshannon? (with a goto to avoid lots of if blocks with different numbers of Py_DECREFs)

I have made the requested changes; please review again

bedevere-bot

Thanks for making the requested changes!

@erlend-aasland, @markshannon: please review the changes made to this pull request.

wjakob

The test_asyncio test fails on Windows (Azure Pipelines). Potentially a fluke/unrelated issue? I don't see how it could be related.

AlexWaygood

The test_asyncio test fails on Windows (Azure Pipelines). Potentially a fluke/unrelated issue? I don't see how it could be related.

It's a known issue that will be fixed by #98704

The changes were made.

encukou

I also have a nitpick, otherwise LGTM.
I'll push a commit to your branch to avoid another round of review ping-pong.

The comment can go away now.

encukou

Thank you for the PR!

wjakob

Awesome, many thanks for your help in getting it merged!

wjakob

I realized that I forgot to add an entry to Doc/whatsnew/3.12.rst. Should I open a separate PR, or will somebody from the maintainers team do this?

markshannon

Open a new PR please.

wjakob requested a review from a team as a code owner October 24, 2022 08:33

bedevere-bot added the awaiting review label Oct 24, 2022

wjakob mentioned this pull request Oct 24, 2022

Add vector call functions to the limited API #98586

Closed

wjakob force-pushed the vectorcall-public branch from abc4964 to 85d09f5 Compare October 24, 2022 08:47

erlend-aasland requested review from encukou, erlend-aasland and vstinner October 24, 2022 10:49

erlend-aasland linked an issue Oct 24, 2022 that may be closed by this pull request

Add vector call functions to the limited API #98586

Closed

erlend-aasland reviewed Oct 24, 2022

View reviewed changes

expose more of the PEP-590 vector call API

7acde40

wjakob force-pushed the vectorcall-public branch from 85d09f5 to 8732fb0 Compare October 24, 2022 13:34

wjakob added 2 commits October 24, 2022 15:36

add testcases to test_capi

68e17a9

adjusted NEWS entry to use Sphinx formatting directives

9934529

wjakob force-pushed the vectorcall-public branch from 8732fb0 to 9934529 Compare October 24, 2022 13:36

markshannon self-requested a review October 24, 2022 14:10

rename internal test methods to avoid them being picked up as indepen… …

35685b0

…dent tests

erlend-aasland changed the title gh-98586: expose more of the PEP-590 vector call API gh-98586: Add vector call APIs to the Limited API Oct 25, 2022

erlend-aasland reviewed Oct 25, 2022

View reviewed changes

Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-24-10-30-30.gh-issue-98586.Tha5Iy.rst Outdated Show resolved Hide resolved

erlend-aasland reviewed Oct 25, 2022

View reviewed changes

Comment thread Modules/_testcapi/vectorcall_limited.c Outdated Show resolved Hide resolved

incorporate feedback

550df59

bedevere-bot added the awaiting changes label Oct 26, 2022

fix potential leaks in test suite

8d05d91

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 26, 2022

bedevere-bot requested review from erlend-aasland and markshannon October 26, 2022 12:54

remove superflous assignment

2c893e0

erlend-aasland reviewed Oct 26, 2022

View reviewed changes

Comment thread Modules/_testcapi/vectorcall_limited.c Outdated Show resolved Hide resolved

Comment thread Modules/_testcapi/vectorcall_limited.c Outdated Show resolved Hide resolved

formatting tweaks

7968a83

Remove outdated commit

72169aa

encukou approved these changes Oct 27, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 27, 2022

encukou merged commit e60892f into python:main Oct 27, 2022

bedevere-bot removed the awaiting merge label Oct 27, 2022

wjakob deleted the vectorcall-public branch October 27, 2022 10:53

gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022

pythongh-98586 : Add vector call APIs to the Limited API (pythonGH-98587 ) …

1ec22b5

Expose the facilities for making vector calls through Python's limited API.

wjakob mentioned this pull request Nov 3, 2022

gh-98586: add whatsnew entry covering limited-API outgoing vector calls, related documentation fix #99056

Merged

erlend-aasland mentioned this pull request Nov 11, 2022

gh-96002: Add functional test for Argument Clinic #96178

Merged

wjakob mentioned this pull request Mar 5, 2024

Implement stable ABI builds nicholasjng/nanobind-bazel#5

Merged

                    * (Empty list left here as template/example, since using
                    * PyModule_AddFunctions isn't very common.)
                    */

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-98586: Add vector call APIs to the Limited API#98587