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

Next steps for the Python API? #116

Closed
seberg opened this issue Dec 13, 2022 · 7 comments · Fixed by #136
Closed

Next steps for the Python API? #116

seberg opened this issue Dec 13, 2022 · 7 comments · Fixed by #136

Comments

@seberg
Copy link
Collaborator

seberg commented Dec 13, 2022

Once gh-113 is finalized, we can extend the Python exchange API to use the new API.

Let me suggest the following API changes (which have been brought up before):

  • Rename the capsule to dltensor_versioned, the "used" name could remain identical, but probably might as well be renamed to used_dltensor_versioned (or dltensorv to be shorter).

    (This prevents crashes if the producer is newer than the other.)

  • Add new keyword argument __dlpack__(..., max_version=(major, minor)) indicating the maximum version supported by the consumer. The producer should use it to achieve backwards compatibility.
    (The producer may ignore this and return a version not understood by the consumer, but it will limit the producers compatibility with older consumers.)

    Consumers may use a try: obj.__dlpack__(max_version=...)/except TypeError: clause to prefer the new version if available.

  • The producer should use (0, 0) as default (whatever we currently have) for now for backcompat (an old consumer will get something they understand).

  • Incompatible versions should raise a BufferError preferably with a message including both requested and passed/supported versions. The producer may, but is not required to issue this error (it can ignore the max_version, but should at least raise an error if legacy was requested and not supported, since that would otherwise give an unclear capsule name error).

All Python libraries should update to the new version as soon as possible, it is encouraged to deprecate support for the old API version at the same time or soon after.

Alternative: We had discussed adding __dlpack_info__ and then passing in version= rather than a max_version=. This scheme is described here: https://github.com/dmlc/dlpack/pull/101/files#diff-851e948c4827ccec1de9b174d011fc92676f2e195ac863c4a055d9cc508575f4R156
I don't mind this scheme, although I feel that renaming the capsule wouldn't hurt for safety even there.

I am sure there are more alternatives, and I wouldn't shy away from larger changes, but I have no clear plan for that.

@tqchen
Copy link
Member

tqchen commented Jan 4, 2023

Agree with dltensor_versioned renaming change.

+1 to deprecation of the old API.

I also like the max_version argument here.

In terms of the versioning requirement, it would be useful to think about possible deprecation scheme and versions supported by the producer -- since producer ideally should not stay with too old files. Personally I think staying K release cycle(say K=2) makes sense given we are moving slowly, with a preference to deprecate the old DLManagedTensor ASAP.

Because DLPackVersion is always in the header, the producer can safely ignore max_version is necessary, and the consumer can still check the version.

@tqchen
Copy link
Member

tqchen commented Jan 4, 2023

cc @wjakob @rgommers @leofang

@seberg
Copy link
Collaborator Author

seberg commented Jan 5, 2023

To take that over from the other issue: I don't care about introducing a new dunder. I suggested doing this, because I had a feeling there was some want to stick with the __dlpack__ "brand" rather than switch to __vdlpack__ long-term.

Yes, in the transition period where some consumers try to fetch the new version but fall back, it would add 200-300ns overhead (on my machine).

@tqchen
Copy link
Member

tqchen commented Jan 24, 2023

I like the idea of switch with __dlpack__ brand. now that #113 is merged, would be great if we can take actions to update the data-api part of the spec

@leofang
Copy link
Collaborator

leofang commented Jan 24, 2023

We'll see if we can get a short window to discuss about this in the meeting tomorrow later this week.

@rgommers
Copy link
Collaborator

Great to see this moving forward! What is proposed here seems good to me; I also like keeping __dlpack__ with max_version over adding a new dunder method.

So to spell it out, this would be the new signature:

__dlpack__(*,
    max_version: Optional[tuple[int, int]]=None,
    stream: Optional[Union[int, Any]] = None
) -> PyCapsule

And we should update the consumer side of all libraries first with:

try:
    x.__dlpack__(max_version=(1, 0))
    # if it succeeds, store info about capsule name being "dltensor_versioned",
    # and needing to set the capsule name to "used_dltensor_versioned"
    # when we're done
except TypeError:
    x.__dlpack__()

And the producer side at the same time by updating to the new signature, and

def __dlpack__(...):
    if max_version is None:
        # Keep and use the DLPack 0.X implementation
        # Note: in >= 2 years from now (but ideally as late as possible), it's okay
        # to raise BufferError here
    else:
        # We get to produce `DLManagedTensorVersioned` now
        if max_version >= our_own_dlpack_version:
            # Consumer understands us, just return a Capsule with our max version
        elif max_version[0] == our_own_dlpack_version[0]:
            # major versions match, we should still be fine here - return our own max version
        else:
            # if we're at a higher major version internally, did we keep an implementation
            # of the older major version around? If so, use that. Else, just return our max
            # version and let the consumer deal with it.

@rgommers
Copy link
Collaborator

I opened data-apis/array-api#602 for this.

leofang added a commit to data-apis/array-api that referenced this issue Feb 9, 2024
* Add versioning support to DLPack APIs

xref dmlc/dlpack#116

* Address review comment, replace ">=2 years" by "from March 2025"

* nit: re-order

* improvements & fixes

* Satisfy linter

* Satisfy linter

* Update src/array_api_stubs/_draft/array_object.py

Co-authored-by: Sebastian Berg <[email protected]>

---------

Co-authored-by: Leo Fang <[email protected]>
Co-authored-by: Athan <[email protected]>
Co-authored-by: Sebastian Berg <[email protected]>
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 a pull request may close this issue.

4 participants