Skip to content
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

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

Merged
merged 10 commits into from
Oct 27, 2022

Conversation

wjakob
Copy link
Contributor

@wjakob wjakob commented Oct 24, 2022

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

@erlend-aasland
Copy link
Contributor

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

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

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 wjakob force-pushed the vectorcall-public branch from 85d09f5 to 8732fb0 Compare October 24, 2022 13:34
@wjakob
Copy link
Contributor Author

wjakob commented Oct 24, 2022

@erlend-aasland Done.

@wjakob wjakob force-pushed the vectorcall-public branch from 8732fb0 to 9934529 Compare October 24, 2022 13:36
@markshannon markshannon self-requested a review October 24, 2022 14:10
@erlend-aasland
Copy link
Contributor

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

@wjakob
Copy link
Contributor Author

wjakob commented Oct 24, 2022

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
Copy link
Contributor

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.

@erlend-aasland 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
@markshannon
Copy link
Member

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.

  1. Calling through PyObject_Vectorcall
  2. 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
Copy link
Contributor Author

wjakob commented Oct 25, 2022

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

wjakob commented Oct 25, 2022

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
Copy link
Contributor Author

wjakob commented Oct 26, 2022

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
Copy link

Thanks for making the requested changes!

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

@wjakob
Copy link
Contributor Author

wjakob commented Oct 26, 2022

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

@AlexWaygood
Copy link
Member

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

@encukou encukou dismissed markshannon’s stale review October 27, 2022 08:55

The changes were made.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 157 to 159
* (Empty list left here as template/example, since using
* PyModule_AddFunctions isn't very common.)
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment can go away now.

@encukou
Copy link
Member

encukou commented Oct 27, 2022

Thank you for the PR!

@wjakob wjakob deleted the vectorcall-public branch October 27, 2022 10:53
@wjakob
Copy link
Contributor Author

wjakob commented Oct 27, 2022

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

gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022
Expose the facilities for making vector calls through Python's limited API.
@wjakob
Copy link
Contributor Author

wjakob commented Nov 3, 2022

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
Copy link
Member

Open a new PR please.

wjakob added a commit to wjakob/cpython that referenced this pull request Nov 3, 2022
PR python#98587 addressing issue python#98586 lacked a "what's new" entry.

While making those changes, I noticed an inconsistency in how
``PY_VECTORCALL_ARGUMENTS_OFFSET`` is declared in the underlying Sphinx
markup when compared to other macro constants like
``Py_TPFLAGS_HAVE_VECTORCALL``. This PR fixes that as well, which should
connect a few currently broken cross-references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add vector call functions to the limited API
7 participants