| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Add PyUnstable API for PEP 523: * PyUnstable_FrameEvalFunction type * PyUnstable_InterpreterState_GetEvalFrameFunc() * PyUnstable_InterpreterState_SetEvalFrameFunc() Keep the old names as deprecated aliases to new names: * _PyFrameEvalFunction * _PyInterpreterState_GetEvalFrameFunc * _PyInterpreterState_SetEvalFrameFunc
|
Do we need tests on PyUnstable API? |
Sorry, something went wrong.
There are tests in test_optimizer: see _testinternalcapi.set_eval_frame_default(). |
Sorry, something went wrong.
Thanks! |
Sorry, something went wrong.
I added tests. |
Sorry, something went wrong.
Co-authored-by: Mikhail Efimov <efimov.mikhail@gmail.com>
There was a problem hiding this comment.
One question but otherwise looks good!
Sorry, something went wrong.
|
I created a C API Working Group decision issue. |
Sorry, something went wrong.
|
|
||
|
|
||
| static PyObject * | ||
| noop_eval(PyThreadState *tstate, struct _PyInterpreterFrame *f, int exc) |
There was a problem hiding this comment.
Doesn't _PyInterpreterFrame, need a public name too? Otherwise you can't quite use the functions without private API.
Sorry, something went wrong.
There was a problem hiding this comment.
To implement an eval function, you should likely use the internal C API (pycore_interpframe.h) to access frame members, you're right. I don't want to add _PyInterpreterFrame structure members to the PyUnstable API, it's too unstable :-D I prefer to leave it in the internal C API.
I don't think that it's worth it to expose struct _PyInterpreterFrame* type in the PyUnstable API. What do you think?
Sorry, something went wrong.
There was a problem hiding this comment.
These functions are rather useless without making _PyInterpreterFrame public. At least should be an opaque type, with a function to upgrade it to the full PyFrameObject.
Sorry, something went wrong.
There was a problem hiding this comment.
The API changed in Python 3.11 to take a _PyInterpreterFrame* instead of a PyFrameObject*. It's basically only used by PyTorch Dynamo which already uses the internal C API:
I wouldn't say that this API is useless without a public API for _PyInterpreterFrame*. It's just "unusual" :-)
At least should be an opaque type, with a function to upgrade it to the full PyFrameObject.
It would make the code (way) slower, _PyInterpreterFrame* idea is to avoid creating a concrete Python object for a frame unless it's strictly needed. I suggest using the internal C API for _PyInterpreterFrame* instead.
Sorry, something went wrong.
There was a problem hiding this comment.
I wouldn't say that this API is useless without a public API for _PyInterpreterFrame*.
What is the use, then?
Sorry, something went wrong.
There was a problem hiding this comment.
@emmatyping @efimov-mikhail: Do you have an opinion on these questions?
Sorry, something went wrong.
There was a problem hiding this comment.
IMO, the main idea of this change is not really to make those functions public. We mark them as PyUnstable_ for sending clear message to its users: "we remember about those function, we don't want to change or remove them in the near future". One can use them and rely on their presence in C API, but can't rely on stability of _PyInterpreterFrame structure. So, there's a need to use some private headers in this case.
I agree that it's not the perfect position for us, but current situation isn't any better.
We have functions that both private and public.
They're private, because they have _Py* names.
But they're "kinda public", since PEP 523 is guaranteed their presence.
PyUnstable_ suits better for those semi-public functions.
But making _PyInterpreterFrame and all its members PyUnstable_ too seems redundant to me.
Sorry, something went wrong.
There was a problem hiding this comment.
But making _PyInterpreterFrame and all its members PyUnstable_ too seems redundant to me.
Same here. I think it should be opaque, and we should add (slow) a function to convert it to the full public PyFrameObject*.
That would make this usable without resorting to private API. Yes, doing that would be slow for the current users, so those will probably continue using the fully-private API -- but that's not the point.
If we expose PyUnstable_InterpreterState_GetEvalFrameFunc,
Or for a different point of view: really the issue solved here is that we don't want documented _Py-prefixed API.
That should apply to argument types too.
Sorry, something went wrong.
There was a problem hiding this comment.
I created #141950 to add PyUnstable_InterpreterFrame_GetFrameObject().
Sorry, something went wrong.
|
PyUnstable_InterpreterState_SetEvalFrameFunc() can now fail, its return type is int (instead of void). |
Sorry, something went wrong.
|
The C API Working Group decided to not add a public function for this feature, but keep the _Py name instead: capi-workgroup/decisions#89. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Add a PyUnstable API for PEP 523:
Keep the old names as deprecated aliases to new names: