Skip to content

Add a C API to assign a new version to a PyTypeObject #103091

Closed
Closed
Add a C API to assign a new version to a PyTypeObject#103091
Labels
topic-C-APItype-featureA feature request or enhancement

Description

@swtaarrs
swtaarrs
opened on Mar 28, 2023

Feature or enhancement

I would like to add an API function like this:

/* Attempt to assign a version tag to the given type.
 *
 * Returns 1 if the type already had a valid version tag or a new one was
 * assigned, or 0 if a new tag could not be assigned.
 */
PyAPI_FUNC(int) _PyType_AssignVersionTag(PyTypeObject* type);

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/MetaPython@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 from PyType_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

Activity

  1. swtaarrs
    added
    type-featureA feature request or enhancement
    on Mar 28, 2023
  2. swtaarrs
    added 2 commits that reference this issue on Mar 28, 2023

    pythongh-103091: Add PyType_AssignVersionTag

    5aade39

    pythongh-103091: Add PyType_AssignVersionTag

    3b329c6
  3. AlexWaygood
    added
    topic-C-API
    on Mar 28, 2023

  4. encukou commented on Mar 31, 2023

    @encukou
    encukou
    on Mar 31, 2023
    Member

    AFAIK, version tags are an implementation detail. For anything except JITs/optimizers, PyType_Modified() should be enough API.
    IMO it would make sense to put this function in the unstable API. Name the function PyUnstable_Type_AssignVersionTag to do that.

  5. carljm commented on Mar 31, 2023

    @carljm
    carljm
    on Mar 31, 2023
    Member

    I suspect it would also make more sense for the declaration to live in Include/cpython/object.h rather than Include/object.h, though I guess this isn't strictly required?

  6. carljm
    added 3 commits that reference this issue on Apr 24, 2023

    gh-103091: Add PyUnstable_Type_AssignVersionTag (#103095)

    b7f4811

    Merge branch 'main' into superopt

    19b8025

    Merge branch 'superopt' into superopt_spec

    1972ca4

  7. swtaarrs commented on Apr 25, 2023

    @swtaarrs
    swtaarrs
    on Apr 25, 2023
    MemberAuthor

    #103095 is merged. Thanks @encukou and @carljm!

  8. swtaarrs
    closed this as completedon Apr 25, 2023

  9. brandtbucher commented on Jun 21, 2023

    @brandtbucher
    brandtbucher
    on Jun 21, 2023
    Member

    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?

  10. carljm commented on Jun 21, 2023

    @carljm
    carljm
    on Jun 21, 2023
    Member

    I'm glad you're finding uses for this API :)

    The PR to expose this function outside typeobject.c just passed through the existing return value convention from the pre-existing staticassign_version_tag function, which has been there for a long time; I don't think changing it was ever considered.

    I'm not sure how consistent the convention is; e.g. the compiler always returns 0 on failure and 1 on success. (EDIT: never mind, looks like this was changed in #100010!) Also, -1 often signifies "exception is set," but this API never sets an exception, it's just indicating whether a version tag was successfully set.

    No strong feelings here.

  11. erlend-aasland commented on Jun 21, 2023

    @erlend-aasland
    erlend-aasland
    on Jun 21, 2023
    Contributor

    Also, -1 often signifies "exception is set," but this API never sets an exception, it's just indicating whether a version tag was successfully set.

    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.

  12. encukou commented on Jun 22, 2023

    @encukou
    encukou
    on Jun 22, 2023
    Member

    There's some discussion general guidelines starting over at #105201 (comment)

    IMO, 0 and 1 is consistent with the direction of that discussion:

    • -1 would means error, with an exception set
    • 0 means "lesser result" (like "no" or "missing value")
    • 1 means "greater result" (like "yes" or "valid value")

    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.

    That's a valid concern -- adding an error case to an existing function, while trying to keep API compatibility, is very messy.
    On the one hand this is unstable API, where we can just remove the function & replace it with a new one.

    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.

  13. carljm commented on Jun 22, 2023

    @carljm
    carljm
    on Jun 22, 2023
    Member

    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.

  14. encukou commented on Jun 22, 2023

    @encukou
    encukou
    on Jun 22, 2023
    Member

    I wonder if there is still room

    Yup, it's totally fine for unstable API, where you can just yank the function. But it should be an informed decision.

  15. markshannon commented on Jun 22, 2023

    @markshannon
    markshannon
    on Jun 22, 2023
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    topic-C-APItype-featureA feature request or enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions