dlpack icon indicating copy to clipboard operation
dlpack copied to clipboard

Future ABI compatibility

Open ryanolson opened this issue 7 years ago • 55 comments

DLPack’s vision of a common in-memory tensor format that spans device and memory types is fantastic.

However, in its form, there is no upgrade path for adding new items to the either the DLTensor or DLManagedTensor structs in way that would maintain ABI compatibility.

I would like to propose the addition of two components to the DLTensor struct. This will break current ABI compatibility, but will in the long run future proof the design.

Additions to DLTensor

  1. unt8/16_t version
  2. uint64_t future_bytes

Adding the version allows the receiving library to determine if the DLTensor can be consumed. The receiver may not have a matching version, but as long as it knows the version it can make a decision on if the data can be correctly used.

Adding future_bytes allows for the addition of new options to DLTensor. One of which might be data layout, ie row-major or column major (c-format vs FORTRAN). I will open a separate issue for this feature.

ryanolson avatar Mar 20 '19 20:03 ryanolson

I feel that we could write additional information in managed_ctx. For example, when borrowing numpy ndarrays, we could save a pointer to a Python object inside the managed_ctx. As long as two framework have protocol about how to interpret the managed_ctx, it will be fine

junrushao avatar Mar 27 '19 21:03 junrushao

Has this been discussed in more detail elsewhere? The lack of a version attribute inside one of the structs is indeed odd, which came up in the conversation around NumPy adopting DLPack. There's #define DLPACK_VERSION 050 in dlpack.h, but that seems to only serve as documentation since it's not accessible by a library receiving a DLPack capsule.

rgommers avatar May 19 '21 14:05 rgommers

I agree, given that we are ABI compatible so far this isn't an issue so far.

How about we allow attaching version information in the capsule for now, this way we do not need to update the struct while still allows libraries to check it.

tqchen avatar May 19 '21 15:05 tqchen

That seems good, cause breaking ABI for it would be painful. Do you mean in the name string of the PyCapsule, or passing it as a second value separately like:

# return version as second argument, `(major, minor, micro)`
def __dlpack__(self, stream=None) -> Tuple[PyCapsule, Tuple[int, int, int]]:

rgommers avatar May 19 '21 16:05 rgommers

we can directly attach it as a version attribute of the PyCapsule

tqchen avatar May 19 '21 17:05 tqchen

It seems PyCapsules cannot accept additional attributes:

>>> handle.version = 5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'PyCapsule' object has no attribute 'version'

This can happen as PyCapsules don't have a __dict__.

hameerabbasi avatar May 24 '21 15:05 hameerabbasi

I see, for backward compat reason with the previously defined protocol, then perhaps dlpack_version is a better way to handle this

tqchen avatar May 24 '21 16:05 tqchen

I'd prefer to avoid a third attribute at the Python level if possible. The two options I suggested still seem feasible:

  • a second return value from __dlpack__
  • Using PyCapsule_SetName and _GetName with some convention like "name must be DLPACK_VERSION=x.y.z".

The first one may be easier to implement (parsing version strings in C is a pain, it typically breaks for a naive implementation once you hit 0.10.0), the second is a bit cleaner from the API perspective.

rgommers avatar May 24 '21 16:05 rgommers

  • a second return value from __dlpack__

I'd prefer this one if at all possible. Also, we should have an ABI version along with a library version.

hameerabbasi avatar May 24 '21 16:05 hameerabbasi

Also, we should have an ABI version along with a library version.

That's actually the only thing we really care about, right?

rgommers avatar May 24 '21 17:05 rgommers

That's actually the only thing we really care about, right?

I'd think so, but currently the version is 050, which I assume means 0.5.0. We can also use the major version to indicate an ABI break.

hameerabbasi avatar May 24 '21 17:05 hameerabbasi

I'd prefer to avoid a third attribute at the Python level if possible.

One advantage of doing this though is that it's more flexible - it can be checked before actually calling DLPack. Especially when streams are involved, it's possible that a consumer calls __dlpack__, then producer inserts a "wait for event" on the stream, then returns the capsule + version, and then the consumer needs to raise.

Another small issue is that TVM (and perhaps some other library?) already implemented support.

So I'm coming around to maybe using __dlpack_version__, which could then use both an API and ABI version.

Would be nice to get some more opinions. @leofang, @emcastillo, @kkraus14?

rgommers avatar May 24 '21 18:05 rgommers

Why are we planning on handling this at the Python level? DLPack is a C header and is usable from C libraries as well. If we solve the problem for C libraries it kinda naturally solves the problem for Python as well.

Can we please avoid Python only solutions?

kkraus14 avatar May 24 '21 18:05 kkraus14

The concern is breaking the ABI, you can't just add a field. Can you think of a way to do it in C in a reasonable manner @kkraus14?

rgommers avatar May 24 '21 18:05 rgommers

One thing I do not quite like about __dlpack_version__ is that it is imaginable that it is not constant. I.e. a data format should probably export it with the lowest compatible version, even if it supports a higher one. The use case is: Some additional information is added to the struct, but only necessary for GPU and not CPU. So NumPy might not support the newer version, so the CPU export uses a lower version than the GPU one.

But, I admit it should be constant for a single tensor/array (although not unimaginable that it is not).

Putting it in the DLPackManagedTensor seems nicest... But yeah, the struct can only be extended if we are OK to crash when the consumer needs it, but the producer is too old. On the up-side, it might be years until most consumers even have a reason to read it, and within the Python ecosystem we could probably standardize quickly on a recent enough DLPack?)

Otherwise, maybe a new __dlpack_info__ struct could work? It could contain the version, device, and any other information we may need in the future. (Replacing the __dlpack_device__) And have a C-counterpart in the DLPack header.

In a sense that would add __dlpack_info__ as an additional "exchange information" next to DLPackManagedTensor. We could then consider having __dlpack__ return a tuple (DLPackManagedTensor, __dlpack_info__), where __dlpack_info__ can be queried on its own for stream support.

seberg avatar May 24 '21 18:05 seberg

The concern is breaking the ABI, you can't just add a field. Can you think of a way to do it in C in a reasonable manner @kkraus14?

It's a 0.x project so we could just break the ABI 😄. Otherwise we'd need to do something similar to what we did with the stream on the Python side and pass it out of band.

kkraus14 avatar May 24 '21 21:05 kkraus14

  • Using PyCapsule_SetName and _GetName with some convention like "name must be DLPACK_VERSION=x.y.z".

@rgommers This is not OK. I think we missed to specify in the Array API standard that a live capsule must have the name "dltensor", which is in fact part of the DLPack protocol since its Day 1. (We did say a dead one is named "used_dltensor".) Capsules must be looked up by name, so we cannot have the name varying across libraries. I can push a PR to array-api for clarification.

Why are we planning on handling this at the Python level? DLPack is a C header and is usable from C libraries as well. If we solve the problem for C libraries it kinda naturally solves the problem for Python as well.

This makes sense to me, although let's not forget the current exchange protocol is fully complete only in Python (via __dlpack__, __dlpack_device__, and from_dlpack()).

I feel it's about time for us to revisit @kkraus14's request #65 --- maybe it is sufficient to "backport" all the Python components to C? Then, we can create a new C function, which could be, say, get_dlpack_info() that backs @seberg's __dlpack_info__ idea to return the version. Adding new functions does not break ABI AFAIK.

A separate question is if returning DLPACK_VERSION alone is sufficient or not. @rgommers You mentioned __dlpack_version__ should return both API and ABI versions. Is a single version not sufficient?

leofang avatar May 25 '21 08:05 leofang

Is a single version not sufficient?

I believe we can just put ABI breaks in a major version if needed, but IMHO it's best to have a separate version for feature support and ABI.

hameerabbasi avatar May 25 '21 10:05 hameerabbasi

A separate question is if returning DLPACK_VERSION alone is sufficient or not. @rgommers You mentioned __dlpack_version__ should return both API and ABI versions. Is a single version not sufficient?

I think both matter. For example, when DLPack was upgraded in PyTorch from 0.2 to 0.4, the question was if that was ABI-compatible. Without a separate ABI version, the only way to know would be to look into the DLPack commit history, or ask someone. It gets worse when you receive a version from another library that is higher than what you implement in your own library - should you raise an exception or not? There's no way to know if that newer version is ABI-compatible or not. Unless you require that any ABI break increases major version indeed. But there's other reasons to increase major version number, so a separate ABI version seems nicer.

rgommers avatar May 25 '21 11:05 rgommers

I feel it's about time for us to revisit @kkraus14's request #65 --- maybe it is sufficient to "backport" all the Python components to C? Then, we can create a new C function, which could be, say, get_dlpack_info() that backs @seberg's __dlpack_info__ idea to return the version. Adding new functions does not break ABI AFAIK.

This makes sense I think - although coupling version info and stream support isn't necessary, those can be done independently.

Right now we don't have a pressing need for other new fields, adding version info is "future proofing". Making breaking changes just to future-proof things seems a bit gratuitous. So a new function sounds good.

It did bring up another question for me. Are there examples of libraries adding dlpack.h to their own public C/C++ API? I'm curious how they do it (e.g. with an unchanged name, or with a prefix). Multiple libraries all exporting the same symbol is not great. So far I've only looked at Python libraries, and they seem to not offer a C/C++ API.

rgommers avatar May 26 '21 06:05 rgommers

I think it would be great to add version info to DLPack. There have been some requests for ABI breaking changes (https://github.com/data-apis/array-api/issues/191, https://github.com/dmlc/dlpack/issues/97). Unless versioning is done, each ABI breaking change will require everyone to update.

we can create a new C function, which could be, say, get_dlpack_info() that backs @seberg's __dlpack_info__ idea to return the version.

Wouldn't this cause symbol conflicts (since get_dlpack_info() will need to be exported somehow) as @rgommers said? I don't see a way around an ABI break. We can add __dlpack_info__ at Python level but, again, ABI breaking changes will still break C/C++ libraries that have integrated support for DLPack. Any suggestions on how versioning can be achieved without an ABI break?

tirthasheshpatel avatar Feb 28 '22 09:02 tirthasheshpatel

I will bring up an alternative once more: Add "request flags" API. That is, you add a new kwarg to __dlpack__, with requests=uint64 (or similar). The consumer can use it to "request" things from the exporter. You could separate them out into two parts: required flags and requested flags:

  • Some flags are "required" (either high 32bit or you pass two values). If the exporter doesn't know what the flag means, they must raise a BufferError.
  • Most flags could probably be "requests", in the sense that the exporter can just ignore them.

As soon as you really want the exporter to pass out additional information, you would add that first "required" flag. We would assume that 32 flags is plenty for the forsee-able future, most likely there will be very few flags, but nobody will have to worry about the how to add something.

The transition will require a try/except by the consumer (if they want to request anything).

The above is of course only for the Python API, I think it makes sense for a C-API/ABI, but I don't think the C part has a defined exchange protocol anyway.

seberg avatar Feb 28 '22 16:02 seberg

I think there are three options that should future proof DLPack against ABI breaks/changes in the future at Python level:

Option 1

Return a 3-tuple from __dlpack__. The third return value is the ABI version.

Option 2

Introduce a __dlpack_info__ dunder method that returns the API and ABI versions.

One problem with this: In the future, when we break the ABI, let's say by adding the readonly flag, previous versions of the Python libraries that supported from_dlpack would start crashing since the ABI has changed and there is no way for them to know __dlpack_info__ exists. To avoid that, along with __dlpack_info__, we will need to add some version info to the exported capsule so that the imports immediately fail due to invalid capsule name.

One solution is to add the ABI version in the capsule name e.g. "dltensor_v0.1". Since a ABI change breaks both forward and backward compatibility, the importer will immediately error out if it doesn't get a capsule that has the same ABI version i.e. any change in ABI version means the array can't be imported. This will cause errors in Python at a much higher level which is much better than crashing.

Option 3

Implement the requests API as @seberg suggested. I think this is more work than the above two options.


I vote for the first option since it is the easiest to do. For the C API, adding a function that returns a API/ABI version and specifying it in the C spec seems like the best option since DLPack had no way before to guard C applications against ABI breaks. Maybe we can proceed with #72 to address this problem at C level.

@tqchen @seberg @leofang @rgommers @mattip Please let me know which options sounds good to you. We can update the spec with whatever options has consensus.

tirthasheshpatel avatar Mar 10 '22 08:03 tirthasheshpatel

I like option 2 given it is minimum change from the previous API. additionally, we continue to check if __dlpack_info__ in the python side (via hasattr, or try/except) to guard against lower ABI ver

tqchen avatar Mar 11 '22 00:03 tqchen

The point of "Option 3" or generally a "request" API (that could also be a max_version) is that it allows the producer to support multiple versions. It offers a solution to the following "user stories":

  • A producer (e.g. cupy) wants to use the new API/ABI. But wants to stay compatible with released versions of consumers (NumPy, pytorch, …). Without a "request" API, cupy has to choose/trade-off:

    • Break compatibility with older consumers.
    • Wait until all/most consumers implement support for both new and old ABI. (And release cadence/timings differ, so this may not be trivial or take longer.)

    We already had a place where this could have been useful: Adding pinned memory devices, cupy could have kept exporting just CPU as a "device" when the consumer only supports an older version.

  • If anyone has ideas for potential additions that could be future requests by the consumer to the exporter, flags could give a way to add it without requiring new kwargs. Adding a kwarg is not to bad, but adding multiple ones if things come up may be tedious.

I believe __dlpack_info__ is also a good solution to the "possible future requests" story. Maybe a better one. Since a consumer could use it to decide which kwargs it may pass/use (i.e. it is possible to check ahead of time, what kwargs are supported when there is more than 1). However, neither "Option 1" nor "Option 2" offer a solution to a producer who would like to support producing multiple ABIs.

It is up to you to decide what you want to support and anticipate. E.g. you have to decide whether or not you care about the "user story" of a produce who wants to support exporting both old and new ABI versions (at least during transition periods).

seberg avatar Mar 13 '22 21:03 seberg

Rather than a full "requests=" API, could we use a "version=" instead? I think it would be much simpler. I am not sure how practical a more general "requests" API would be, since in practice exporters could not change the readonly or alignment flags without copying the data, which invalidates the whole point of dlpack as a no-copy interchange format. By specifying a version of the API with new fields, the importer could check those fields and error out when conditions for use of the exported object are not met. There could be some negotiation between the importer and exporter about the maximum version supported, and the importer can decide whether that version is "good enough" (and maybe warn) or error out if not.

I would still prefer if the capsule name reflected the version of the data exported, to enhance visibility and debugging (so both option 2 and option 3).

mattip avatar Mar 16 '22 09:03 mattip

Agreed. requests are only needed if the request is not very cheap for some reason. The only use-case I can think of right now is catered to supporting producers who want to implement __dlpack__ but must copy to do so. That seems not the target audience for the foreseeable future, and I don't have other thoughts. So I do not care either way.

I agree with Matti on renaming the struct: It keeps all ABI related things firmly in C, rather than a mix of C and Python. In the future, I don't know if renames are necessary. I assumed the ABI and API version will be the first field(s) in the new struct, and probing that seems more convenient to me then probing the capsule name (maybe not for debugging?).

seberg avatar Mar 16 '22 15:03 seberg

A few thoughts:

  • I think a separate struct is needed at least, so Option 2 in both C (get_dlpack_info()) and Python (__dlpack_info__)
  • The Option 3 "request a version" to be able to provide two or more versions and enable a smooth upgrade path makes sense to me as well, and is not exclusive with Option 2. I think it's okay for this to be Python-only; the Python ecosystem is probably the only place where it's going to be used.
  • A reminder that both DLPack version and ABI version may matter, so just version= is unclear.
    • Examples:
      • Adding a readonly flag is extra metadata only and backwards-compatible (given that we want readonly arrays to be exported today), so will not require an ABI version bump.
      • Adding an endianness flag (not desired, but useful as an example) is ABI-breaking, because it changes the semantics compared to today (the producer no longer has to raise). Consumers not aware of it may silently start consuming non-native-ordered arrays if the ABI version would not be bumped.
    • Therefore a "request version" API needs a bit of thought. I can imagine one wants "DLPack version 0.7 or higher", or "ABI version 1".
  • Capsule name is part of the protocol as @leofang pointed out in https://github.com/dmlc/dlpack/issues/34#issuecomment-847669399. Given that it's not necessary for anything and we have two version numbers to care about here, I think this proposal is not a good idea. Let's leave it as "dltensor" and "used_dltensor".

My preference would be to implement Option 2 in one PR, and see a separate follow-up PR to explore Option 3 and select different versions.

rgommers avatar Mar 16 '22 15:03 rgommers

  • Adding a readonly flag is extra metadata only and backwards-compatible (given that we want readonly arrays to be exported today), so will not require an ABI version bump

Do you propose to pass the readonly flag using something like __dlpack_info__? If not, this is a ABI break since we need the DLTensor struct to have the readonly field. If yes, I don't think it's a good choice keeping C applications in mind (but as you said, this might not be used as much in C)

  • A reminder that both DLPack version and ABI version may matter, so just version= is unclear

It could be a tuple: version=(api_version, abi_version)

My preference would be to implement Option 2 in one PR, and see a separate follow-up PR to explore Option 3 and select different versions

Sounds good. I will propose a PR to add the API and ABI versions of DLPack. In a follow-up, I will add the version= parameter and __dlpack_info__ in the spec.

tirthasheshpatel avatar Mar 17 '22 03:03 tirthasheshpatel

If not, this is a ABI break since we need the DLTensor struct to have the readonly field.

It looks to me like that would naturally fit at the end of DLTensor. Note that adding new fields at the end of a struct is not an ABI break (unless it changes the semantics of other fields, which readonly doesn't); older code that is unaware of the new field will continue to work unchanged.

rgommers avatar Mar 18 '22 20:03 rgommers