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

[ABI break] Append new fields to DLManagedTensor #105

Closed

Conversation

tirthasheshpatel
Copy link
Contributor

Alternative to #101
Addresses #34 and #104
Implements the A1 alternative in #104 (comment)

Instead of appending the new fields to the DLTesor which changes the layout of DLManagedTensor in a backward incompatible manner, we can add the fields at the end of DLManagedTensor itself. This way we won't require new structs to maintain backward compatibility.

On Python front: should we add the __dlpack_info__ dunder and a version keyword as proposed in #101? I'd say yes because, without knowing the requested version, the producer (exporter) cannot tell if it can export a certain type of array e.g. NumPy throws an error if one tries to export a readonly array with the old ABI. On the consumer (importer) side, it can't tell which fields are safe to access without knowing what version is being imported.

cc @tqchen @mattip @seberg @rgommers @leofang

@@ -222,6 +222,12 @@ typedef struct DLManagedTensor {
* The destructors deletes the argument self as well.
*/
void (*deleter)(struct DLManagedTensor * self);
/* The version of the DLManagedTensor */
int32_t version;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing a new struct with a DLPack and ABI version, we can just export the DLPack version since it uniquely determines the ABI version. (cc @mattip)

/*! \brief Mark the data readonly. */
uint8_t readonly;
/*! \brief 128-bytes of padding to preserve ABI compatibility in the future. */
uint8_t __padding[128];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 128 bytes of padding enough (or is it too much)?

@mattip
Copy link

mattip commented Apr 15, 2022

I think this need to include the __dlpack_info__ parts of #101 and to provide two structs: a v1 DLManagedTensor and a v2+ DLManagedTensorVersioned. The default will be to return a DLManagedTensor. If the consumer requests a higher version we can return a DLManagedTensorVersioned. Then it is the consumer's responsibility to check the extra fields iff they requested the higher version.

@tirthasheshpatel
Copy link
Contributor Author

I think this need to include the __dlpack_info__ parts of #101 and to provide two structs: a v1 DLManagedTensor and a v2+ DLManagedTensorVersioned. The default will be to return a DLManagedTensor. If the consumer requests a higher version we can return a DLManagedTensorVersioned. Then it is the consumer's responsibility to check the extra fields iff they requested the higher version.

I thought the reason to append new fields to DLManagedTensor itself was to avoid the requirement of two different structs. Since the layout of DLManagedTensor or DLTensor doesn't change, there should not be any breaks in existing code if the same struct is exported with extra fields. Although, I agree we should add the __dlpack_info__ and version keyword (as we did in #101) so importers know what fields are safe to access and exporters know if they can export a certain type of array.

@tqchen
Copy link
Member

tqchen commented Jan 5, 2023

closed in favor of #113

We would like to thanks @tirthasheshpatel for your effort on bringing in this change onward

@tqchen tqchen closed this Jan 5, 2023
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 this pull request may close these issues.

None yet

3 participants