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.

Sign up for GitHub

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] ABI Update For adding Version to DLPack #113

Merged
merged 4 commits into from
Jan 24, 2023
Merged

[ABI] ABI Update For adding Version to DLPack #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecb83f4
[ABI] ABI Update For adding Version to DLPack
tqchen Nov 9, 2022
8af4a52
Address comments.
tqchen Nov 13, 2022
73dd3d5
Add proposed major/minor version change
tqchen Jan 4, 2023
d10634c
More clarifications
tqchen Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions contrib/mock_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
#include <dlpack/dlpack.h>
#include <dlpack/dlpackcpp.h>

int CheckFlags(DLManagedTensorVersioned *data) {
if (data->flags & DLPACK_FLAG_BITMASK_READ_ONLY) {
return 0;
} else {
return 1;
}
}

int main() {
dlpack::DLTContainer c;
return 0;
Expand Down
10 changes: 9 additions & 1 deletion docs/source/c_api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ Macros

.. doxygendefine:: DLPACK_EXTERN_C

.. doxygendefine:: DLPACK_VERSION
.. doxygendefine:: DLPACK_MAJOR_VERSION

.. doxygendefine:: DLPACK_MINOR_VERSION

.. doxygendefine:: DLPACK_DLL

Expand All @@ -22,6 +24,9 @@ Enumerations
Structs
~~~~~~~

.. doxygenstruct:: DLPackVersion
:members:

.. doxygenstruct:: DLDevice
:members:

Expand All @@ -33,3 +38,6 @@ Structs

.. doxygenstruct:: DLManagedTensor
:members:

.. doxygenstruct:: DLManagedTensorVersioned
:members:
102 changes: 94 additions & 8 deletions include/dlpack/dlpack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
#define DLPACK_EXTERN_C
#endif

/*! \brief The current version of dlpack */
#define DLPACK_VERSION 80
/*! \brief The current major version of dlpack */
#define DLPACK_MAJOR_VERSION 1

/*! \brief The current ABI version of dlpack */
#define DLPACK_ABI_VERSION 1
/*! \brief The current minor version of dlpack */
#define DLPACK_MINOR_VERSION 0

/*! \brief DLPACK_DLL prefix for windows */
#ifdef _WIN32
Expand All @@ -38,6 +38,33 @@
#ifdef __cplusplus
extern "C" {
#endif

/*!
* \brief The DLPack version.
*
* A change in major version indicates that we have changed the
* data layout of the ABI - DLManagedTensorVersioned.
*
* A change in minor version indicates that we have added new
* code, such as a new device type, but the ABI is kept the same.
Copy link

Choose a reason for hiding this comment

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

I would add something of the following sort just to spell it out clearly somewhere:

If an obtained DLPack tensor has a major version that disagrees with the version number specified in this header file (i.e. major != DLPACK_MAJOR_VERSION), the consumer must call the deleter. It is not safe to access any other fields as the memory layout will have changed. In the case of a minor version mismatch, the tensor can be safely used as long as the consumer knows how to interpret all fields. Minor version updates indicate the addition of enumeration values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the great suggestion, just add the comment per suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

Great suggestion +1

*
* If an obtained DLPack tensor has a major version that disagrees
* with the version number specified in this header file
* (i.e. major != DLPACK_MAJOR_VERSION), the consumer must call the deleter
* (and it is safe to do so). It is not safe to access any other fields
* as the memory layout will have changed.
*
* In the case of a minor version mismatch, the tensor can be safely used as
* long as the consumer knows how to interpret all fields. Minor version
* updates indicate the addition of enumeration values.
*/
typedef struct {
/*! \brief DLPack major version. */
uint32_t major;
/*! \brief DLPack minor version. */
uint32_t minor;
} DLPackVersion;

/*!
* \brief The device type in DLDevice.
*/
Expand Down Expand Up @@ -211,6 +238,13 @@ typedef struct {
* not meant to transfer the tensor. When the borrowing framework doesn't need
* the tensor, it should call the deleter to notify the host that the resource
* is no longer needed.
*
* \note This data structure is used as Legacy DLManagedTensor
* in DLPack exchange and is deprecated after DLPack v0.8
tqchen marked this conversation as resolved.
Show resolved Hide resolved
* Use DLManagedTensorVersioned instead.
* This data structure may get renamed or deleted in future versions.
*
* \sa DLManagedTensorVersioned
*/
typedef struct DLManagedTensor {
/*! \brief DLTensor which is being memory managed */
Expand All @@ -219,13 +253,65 @@ typedef struct DLManagedTensor {
* which DLManagedTensor is used in the framework. It can also be NULL.
*/
void * manager_ctx;
/*! \brief Destructor signature void (*)(void*) - this should be called
* to destruct manager_ctx which holds the DLManagedTensor. It can be NULL
* if there is no way for the caller to provide a reasonable destructor.
* The destructors deletes the argument self as well.
/*!
* \brief Destructor - this should be called
Copy link

@wjakob wjakob Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit picking: formatting is a little messed up (double spaces, rows of different length).

Edit: That also applies to a few of the other newly added comments, but I will just mention it once here to avoid spamming the PR.

Also: I think the idea of the doxygen \brief tag is that the description is actually brief (1 sentence) followed by a newline to separate it from a more thorough comment. The \brief part is used as a preview.

Copy link
Member Author

Choose a reason for hiding this comment

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

great point, fixed for this particular case. we can do further cleanups

* to destruct the manager_ctx which backs the DLManagedTensor. It can be
* NULL if there is no way for the caller to provide a reasonable destructor.
* The destructors deletes the argument self as well.
*/
void (*deleter)(struct DLManagedTensor * self);
} DLManagedTensor;

// bit masks used in in the DLManagedTensorVersioned

/*! \brief bit mask to indicate that the tensor is read only. */
#define DLPACK_FLAG_BITMASK_READ_ONLY (1UL << 0UL)

/*!
* \brief A versioned and managed C Tensor object, manage memory of DLTensor.
*
* This data structure is intended to facilitate the borrowing of DLTensor by
* another framework. It is not meant to transfer the tensor. When the borrowing
* framework doesn't need the tensor, it should call the deleter to notify the
* host that the resource is no longer needed.
*
* \note This is the current standard DLPack exchange data structure.
*/
struct DLManagedTensorVersioned {
/*!
* \brief The API and ABI version of the current managed Tensor
*/
DLPackVersion version;
tqchen marked this conversation as resolved.
Show resolved Hide resolved
/*!
* \brief the context of the original host framework.
*
* Stores DLManagedTensorVersioned is used in the
* framework. It can also be NULL.
*/
void *manager_ctx;
/*!
* \brief Destructor.
*
* This should be called to destruct manager_ctx which holds the DLManagedTensorVersioned.
* It can be NULL if there is no way for the caller to provide a reasonable
* destructor. The destructors deletes the argument self as well.
*/
void (*deleter)(struct DLManagedTensorVersioned *self);
/*!
* \brief Additional bitmask flags information about the tensor.
*
* By default the flags should be set to 0.
*
* \note Future ABI changes should keep everything until this field
* stable, to ensure that deleter can be correctly called.
*
* \sa DLPACK_FLAG_BITMASK_READ_ONLY
*/
uint64_t flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to note somewhere that we guarantee the fields remain ABI stable until here (I guess including the flags).
That is because consumers must be able to call the destructor even if they don't understand the version.

But after this, everything may be version dependent (it may make sense to specify whether growing the struct at the end is considered valid without bumping ABI in theory).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree

Copy link

@wjakob wjakob Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is the key part, and it would be useful to summarize the high-level idea in this header since it's the most important file of the entire repository.

If we implement DLPack version X and get a tensor of version Y (where Y>X), what are we allowed to do? What are we not allowed to do?

Copy link

@wjakob wjakob Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There are also two version numbers: the DLpack version, and the ABI version. Is the first one completely irrelevant in deciding what is valid/disallowed, or does it also play a potential role? If yes, how is it different from the ABI version? If no, why does it need to be part of the ABI via a field stored in data structures?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be a major.minor and minor allows e.g. additional (compatible!) flags or additional devices/dtypes (not strictly compatible, but clearly can't fail). Not sure what the current scheme is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some clarification of the current scheme

/*! \brief DLTensor which is being memory managed */
DLTensor dl_tensor;
};

#ifdef __cplusplus
} // DLPACK_EXTERN_C
#endif
Expand Down