FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-91744: Add semi-stable C API tier by encukou · Pull Request #91789 · python/cpython · GitHub

Repository navigation

gh-91744: Add semi-stable C API tier - #91789

Closed
encukou wants to merge 14 commits into
python:mainfrom
encukou:semi-stable
Closed

encukou wants to merge 14 commits into
python:mainfrom
encukou:semi-stable

Conversation

encukou commented Apr 21, 2022

Copy link
Copy Markdown
Member

This adds the semi-stable API tier witth a few initial functions/types. Adding more is left to another PR.

encukou marked this pull request as draft April 21, 2022 16:07
encukou mentioned this pull request Apr 21, 2022
8 of 19 tasks

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is PyInterpreterState_SetEvalFrameFunc() really usable in the semi-stable API? The second parameter type comes from the internal C API. There is nothing to inspect a _PyInterpreterFrame structure in the semi-stable API.

I don't know how debuggers use PyInterpreterState_SetEvalFrameFunc().

cc @markshannon

Copy link
Copy Markdown
Member

See also #91788

Copy link
Copy Markdown
Member

See also #91906

h-vetinari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some comments from taking a look out of curiosity

Comment thread Include/README.rst Outdated
Comment thread Doc/whatsnew/3.11.rst Outdated
Comment thread Doc/c-api/stable.rst Outdated
Comment on lines +21 to +27
There are two tiers of API with different stability exepectations,
enabled by specific macros:

- :c:macro:`Py_USING_SEMI_STABLE_API` exposes API that may change
without deprecation warnings.
- :c:macro:`Py_LIMITED_API` exposes API that is compatible across
several minor releases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

exepectations -> expectations

One could also say: "there are two public tiers of the API" (emphasis just to show addition); underscored resp. Py_BUILD_CORE are arguably a tier as well...?

I've struck out the "two" because I think it would be clearer to expose the full hierarchy, à la:

Suggested change
There are two tiers of API with different stability exepectations,
enabled by specific macros:
- :c:macro:`Py_USING_SEMI_STABLE_API` exposes API that may change
without deprecation warnings.
- :c:macro:`Py_LIMITED_API` exposes API that is compatible across
several minor releases.
There are three public tiers of the API with different stability
expectations, two of which need to be enabled by specific macros:
- :c:macro:`Py_LIMITED_API` exposes API that is compatible across
several minor releases.
- (no macro): default C-API covered by :pep:`387`.
- :c:macro:`Py_USING_SEMI_STABLE_API` exposes API that may change
without deprecation warnings between minor releases.

I'd also suggest to lead with the most stable one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

By “There are two tiers of API with different stability expectations” I wanted to present them as being additional, different from the default that's covered in the preceding paragraphs.
The order matches the detailed sections below, which would be somewhat tricky to reorder -- the one-paragraph semi-stable tier would feel lost if it was at the end.

Comment thread Doc/c-api/stable.rst Outdated
Comment thread Doc/c-api/code.rst Outdated
Comment thread Doc/c-api/code.rst Outdated
use :c:func:`PyCode_NewEmpty` instead.

Since the definition of the bytecode changes often, calling
:c:func:`PyCode_New` directly can bind you to a precise Python version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

style: better "can directly" than "directly can" (or directly at the very end)

encukou Apr 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's not “can directly bind you”, but “calling :c:func:PyCode_New directly” (as opposed to PyCode_Replace for example)

@@ -0,0 +1,10 @@
Defining :c:macro:`Py_USING_SEMI_STABLE_API` will expose "semi-stable API",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps it's worth considering to stay consistent with the spelling of "semi-stable" / "semistable". Currently the include folders are named like the latter (which I think looks good in a path), mentions in the docs like here often have the hyphen.

Even though the hyphen is grammatically slightly more correct, my personal preference would be without.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Which also opens the question if the macro should be Py_USING_SEMI_STABLE_API or Py_USING_SEMISTABLE_API

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I still have hope that someone will come up with a better name. Python-dev is usually so good at bikeshedding!
I'll postpone renaming for now.

encukou commented Apr 28, 2022

Copy link
Copy Markdown
Member Author

Thank you for the comments!

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I expected the API and macro to mention "unstable" rather than "semi-stable" (Py_USING_SEMI_STABLE_API => Py_USING_UNSTABLE_API).

Comment thread Include/cpython/code.h
PyObject *, PyObject *, PyObject *, int, PyObject *,
PyObject *);
/* same as struct above */
/* See Include/semi-stable/code.h for PyCode_New* */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure that this comment is useful.

Suggested change
/* See Include/semi-stable/code.h for PyCode_New* */
/* See Include/unstable/code.h for PyCode_New* */

Comment thread Include/pyport.h

// Expose unstable API when building Python
#ifdef Py_BUILD_CORE
#define Py_USING_UNSTABLE_API

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
#define Py_USING_UNSTABLE_API
# define Py_USING_UNSTABLE_API

Comment thread Include/pyport.h
Comment on lines +381 to +385
#ifdef Py_USING_UNSTABLE_API
#define _Py_NEWLY_UNSTABLE(VERSION_UNUSED)
#else
#define _Py_NEWLY_UNSTABLE(VERSION) Py_DEPRECATED(VERSION)
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
#ifdef Py_USING_UNSTABLE_API
#define _Py_NEWLY_UNSTABLE(VERSION_UNUSED)
#else
#define _Py_NEWLY_UNSTABLE(VERSION) Py_DEPRECATED(VERSION)
#endif
#ifdef Py_USING_UNSTABLE_API
# define _Py_NEWLY_UNSTABLE(VERSION_UNUSED)
#else
# define _Py_NEWLY_UNSTABLE(VERSION) Py_DEPRECATED(VERSION)
#endif

Comment thread Include/unstable/code.h
PyObject *, PyObject *, PyObject *, PyObject *,
PyObject *, PyObject *, PyObject *, int, PyObject *,
PyObject *);
/* same as struct PyCodeObject */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I suggest to move the comment before the function definition: "same parameters as PyCodeObject structure members".


/* Frame evaluation API (added for PEP-523) */

struct _PyInterpreterFrame;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this needed?

Comment thread Doc/c-api/stable.rst

- :c:macro:`Py_USING_SEMI_STABLE_API` exposes API that may change
without deprecation warnings.
- :c:macro:`Py_LIMITED_API` limits exposed API to API that is compatible

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is API repeated on purpose?

Suggested change
- :c:macro:`Py_LIMITED_API` limits exposed API to API that is compatible
- :c:macro:`Py_LIMITED_API` limits exposed API that is compatible

Comment thread Include/cpython/code.h

#define Py_UNSTABLE_CODE_H
#include "unstable/code.h"
#undef Py_UNSTABLE_CODE_H

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would prefer to include it in top-level Include/code.h, as done in Include/pystate.h.

Comment thread Doc/whatsnew/3.11.rst
- :c:func:`PyInterpreterState_GetEvalFrameFunc`
- :c:func:`PyInterpreterState_SetEvalFrameFunc`
- :c:type:`PyFrameEvalFunction`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe mention also _PyFrameEvalFunction type which is renamed?

encukou commented Aug 10, 2022

Copy link
Copy Markdown
Member Author

I'm deferring the PEP. Thanks for the comments, I'll take them into account next time I try.

encukou closed this Aug 10, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL