-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Add a C API to assign a new version to a PyTypeObject #103091
Comments
AFAIK, version tags are an implementation detail. For anything except JITs/optimizers, |
I suspect it would also make more sense for the declaration to live in |
* main: (53 commits) pythongh-102498 Clean up unused variables and imports in the email module (python#102482) pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244) pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365) pythongh-94300: Update datetime.strptime documentation (python#95318) pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778) pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456) pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775) Revert "Add tests for empty range equality (python#103751)" (python#103770) pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519) pythonGH-65022: Fix description of copyreg.pickle function (python#102656) pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324) pythongh-91687: modernize dataclass example typing (python#103773) pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747) pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769) pythongh-87452: Improve the Popen.returncode docs Removed unnecessary escaping of asterisks (python#103714) pythonGH-102973: Slim down Fedora packages in the dev container (python#103283) pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095) Add tests for empty range equality (python#103751) pythongh-103712: Increase the length of the type name in AttributeError messages (python#103713) ...
* superopt: (82 commits) pythongh-101517: fix line number propagation in code generated for except* (python#103550) pythongh-103780: Use patch instead of mock in asyncio unix events test (python#103782) pythongh-102498 Clean up unused variables and imports in the email module (python#102482) pythongh-99184: Bypass instance attribute access in `repr` of `weakref.ref` (python#99244) pythongh-99032: datetime docs: Encoding is no longer relevant (python#93365) pythongh-94300: Update datetime.strptime documentation (python#95318) pythongh-103776: Remove explicit uses of $(SHELL) from Makefile (pythonGH-103778) pythongh-87092: fix a few cases of incorrect error handling in compiler (python#103456) pythonGH-103727: Avoid advancing tokenizer too far in f-string mode (pythonGH-103775) Revert "Add tests for empty range equality (python#103751)" (python#103770) pythongh-94518: Port 23-argument `_posixsubprocess.fork_exec` to Argument Clinic (python#94519) pythonGH-65022: Fix description of copyreg.pickle function (python#102656) pythongh-103323: Get the "Current" Thread State from a Thread-Local Variable (pythongh-103324) pythongh-91687: modernize dataclass example typing (python#103773) pythongh-103746: Test `types.UnionType` and `Literal` types together (python#103747) pythongh-103765: Fix 'Warning: py:class reference target not found: ModuleSpec' (pythonGH-103769) pythongh-87452: Improve the Popen.returncode docs Removed unnecessary escaping of asterisks (python#103714) pythonGH-102973: Slim down Fedora packages in the dev container (python#103283) pythongh-103091: Add PyUnstable_Type_AssignVersionTag (python#103095) ...
I was just bit by this API's somewhat odd return value. Typically we return 0 on success and -1 on failure, but this returns 1 on success and 0 on failure. Perhaps it should be changed? |
I'm glad you're finding uses for this API :) The PR to expose this function outside
No strong feelings here. |
What if we need to change the implementation in the future (because of runtime changes or whatever) and we end up having to use an internal API that can set an exception? If we design this API to always succeed, we have two options: swallow the exception before returning, or add a new API that can return with an exception set. See also the issue about infallible APIs in the C API workgroup repo. |
There's some discussion general guidelines starting over at #105201 (comment) IMO, 0 and 1 is consistent with the direction of that discussion:
That's a valid concern -- adding an error case to an existing function, while trying to keep API compatibility, is very messy. On the other hand, it's not that hard to do: put a note in the docs that it "returns -1 with an exception set" on error, and require that callers do error checking -- even though the exception currently never happens. |
This makes sense in general, but I wonder if there is still room, particularly for an explicitly unstable API, for deeming the likelihood that an API will ever need to raise an exception so low that the practical cost-benefit analysis does not favor making every caller add an extra case for handling something that is currently impossible and probably will never happen in the future, either. Of course anything is possible in the future, but for an unstable API in the unlikely scenario we have the option of just replacing the API. Assigning a version tag to a type is a low-level internal operation (the target audience for its exposure is really only implementers of third-party runtime optimizers / JITs), and a relatively simple one, and I think we would go to some lengths to ensure that it remains simple and doesn't set exceptions in the future. |
Yup, it's totally fine for unstable API, where you can just yank the function. But it should be an informed decision. |
This should never set an exception. Since it can fail (returning 0) and failure doesn't indicate an error, we can just avoid code paths that can raise. |
Feature or enhancement
I would like to add an API function like this:
It would be a thin wrapper around the existing
assign_version_tag
function, and is modeled after a similar function we added to Cinder as part of facebookincubator/cinder@43c4e2d.Pitch
Cinder (and possibly other JITs or optimizing interpreters) would benefit from being able to ensure that type has a valid version tag without having to call
_PyType_Lookup()
just for the version-assigning side-effect. We rely on notifications fromPyType_Modified()
to invalidate code in our JIT when types change, and this only works when the types in question have valid version tags.Previous discussion
There's no written discussion I'm aware of, but @carljm and @markshannon have discussed this offline.
Linked PRs
The text was updated successfully, but these errors were encountered: