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

Update dlpack to v1.0rc #1667

Merged
merged 18 commits into from
May 13, 2024
Merged

Update dlpack to v1.0rc #1667

merged 18 commits into from
May 13, 2024

Conversation

ndgrigorian
Copy link
Collaborator

@ndgrigorian ndgrigorian commented May 9, 2024

This PR bumps the version of dlpack by bringing in the new dlpack.h from v1.0rc.

As a result of changes introduced in v1.0rc

  • get_build_dlpack_version now returns a tuple giving the major and minor DLPack version dpctl was built with
  • support for the new DLManagedTensorVersioned struct has been added, including full support of the new flags attribute
  • __dlpack__ method of usm_ndarray has been updated to choose between DLManagedTensorVersioned and DLManagedTensor paths based on the new max_version keyword argument, and also now has dl_device and copy keyword arguments
  • from_dlpack method has been updated with device and copy keyword arguments and now has a path for arrays supporting the new __dlpack__ interface and DLManagedTensorVersioned. from_dlpack also honors the read-only bitmask that can now be present on a DLManagedTensorVersioned object
  • an enumerator class for the DLPack device types has been added to _usmarray.pyx

This pull request also slips in a minor change to usm_ndarray's constructor to honor the writable flag when buffer is another usm_ndarray

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • If this PR is a work in progress, are you opening the PR as a draft?

Copy link

github-actions bot commented May 9, 2024

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

Copy link

github-actions bot commented May 9, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_336 ran successfully.
Passed: 884
Failed: 8
Skipped: 92

@coveralls
Copy link
Collaborator

coveralls commented May 9, 2024

Coverage Status

coverage: 87.948% (+0.06%) from 87.892%
when pulling f0ced4e on feature/dlpack-1-support
into d3e124a on master.

@ndgrigorian
Copy link
Collaborator Author

@oleksandr-pavlyk
The commit from the memory work branch seems to have broken the dlpack tests on Windows.

Copy link

github-actions bot commented May 9, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_337 ran successfully.
Passed: 883
Failed: 9
Skipped: 92

Copy link

github-actions bot commented May 9, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_339 ran successfully.
Passed: 884
Failed: 8
Skipped: 92

Copy link

github-actions bot commented May 9, 2024

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_340 ran successfully.
Passed: 883
Failed: 9
Skipped: 92

Use `ary_base` rather than `usm_ary` as manager_ctx in
     DLManagedTensor struct.

Tests for DLPack use fixture to provide all devices to take advantage
of caching.
* Uses `dlpack.h` as per DLPack v1.0 rc
* Implements `device` and `copy` keywords into `__dlpack__` and `from_dlpack`
* Implements `max_version` argument into `__dlpack__`
* Adds `DLDeviceType` enumerator to `_usmarray.pyx` for representing device types as enumerators
* Reimplements `dpt._dlpack.get_build_dlpack_version` to return a tuple of the major version and the minor version
…ck_versioned_capsule`

Also adjusts some error messages in _dlpack.pyx
These tweaks are based on similar changes made for the managed tensor during memory work
@ndgrigorian ndgrigorian force-pushed the feature/dlpack-1-support branch from 6920790 to c4b34e5 Compare May 10, 2024 02:34
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_340 ran successfully.
Passed: 884
Failed: 8
Skipped: 92

@oleksandr-pavlyk
Copy link
Collaborator

The check need tweaking:

In [9]: x.__dlpack__(max_version=1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[9], line 1
----> 1 x.__dlpack__(max_version=1)

File ~/repos/dpctl_tmp/dpctl/tensor/_usmarray.pyx:1131, in dpctl.tensor._usmarray.usm_ndarray.__dlpack__()
   1129 else:
   1130     dpctl_dlpack_version = get_build_dlpack_version()
-> 1131     if max_version >= dpctl_dlpack_version or max_version[0] == dpctl_dlpack_version[0]:
   1132         # DLManagedTensorVersioned path
   1133         # TODO: add logic for targeting a device

TypeError: '>=' not supported between instances of 'int' and 'tuple'

The check should check that max_version is a 2-tuple of integers. But looking at this check, what is the purpose of overriding clause max_version[0] == dpctl_dlpack_version[0]?

@oleksandr-pavlyk
Copy link
Collaborator

Please update dpctl/tensor/include/dlpack/README.md with version and SHA of the vendored dlpack.h

@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request May 10, 2024
6 tasks
@ndgrigorian
Copy link
Collaborator Author

The check need tweaking:

In [9]: x.__dlpack__(max_version=1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[9], line 1
----> 1 x.__dlpack__(max_version=1)

File ~/repos/dpctl_tmp/dpctl/tensor/_usmarray.pyx:1131, in dpctl.tensor._usmarray.usm_ndarray.__dlpack__()
   1129 else:
   1130     dpctl_dlpack_version = get_build_dlpack_version()
-> 1131     if max_version >= dpctl_dlpack_version or max_version[0] == dpctl_dlpack_version[0]:
   1132         # DLManagedTensorVersioned path
   1133         # TODO: add logic for targeting a device

TypeError: '>=' not supported between instances of 'int' and 'tuple'

The check should check that max_version is a 2-tuple of integers. But looking at this check, what is the purpose of overriding clause max_version[0] == dpctl_dlpack_version[0]?

max_version[0] >= dpctl_dlpack_version[0] should be enough.

The spec recommends that if the major versions match, it's a strong enough condition to return a capsule with our own max_version.

@ndgrigorian ndgrigorian marked this pull request as ready for review May 10, 2024 19:04
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_344 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

* Fixture all_root_devices is to keep only 2 devices from each platform

This is done to speed-up test suite execution on multi-GPU system,
like Aurora.

* Add tests for legacy/versions capsule import-export

* Expand tests more to improve coverage

Use F-contiguous array to both legacy and versioned capsule.
Add tests for read-only array and for use of copy=True keyword
for __dlpack__ call
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_345 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_346 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

Also adds a test checking for the writable flag of a usm_ndarray constructed with a read-only usm_ndarray as the buffer

Removes some commented out `flags` checks from `test_meshgrid2`. These checks were malformed, as `dpt.meshgrid` returns a non-contiguous view in the test.
@ndgrigorian ndgrigorian force-pushed the feature/dlpack-1-support branch from 99f03ce to 85f12d0 Compare May 10, 2024 23:10
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_346 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

@oleksandr-pavlyk
Copy link
Collaborator

It would be good to factor out common code from to_dlpack_capsule and to_dlpack_versioned_capsule into a function.

Reuse that in both capsule producers to avoid code duplication
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_348 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

oleksandr-pavlyk and others added 3 commits May 12, 2024 15:32
* Reused get_parent_device_ordinal_id routine

* test_legacy_dlpack_capsule uses 4 kinds of dtype

Added test to use non-default copy keyword, and non-default device keyword
argument.
Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_349 ran successfully.
Passed: 889
Failed: 16
Skipped: 91

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_351 ran successfully.
Passed: 888
Failed: 17
Skipped: 91

Copy link

Array API standard conformance tests for dpctl=0.17.0dev0=py310h15de555_352 ran successfully.
Passed: 889
Failed: 16
Skipped: 91

Copy link
Collaborator

@oleksandr-pavlyk oleksandr-pavlyk left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @ndgrigorian

@ndgrigorian ndgrigorian merged commit 8b42313 into master May 13, 2024
57 of 58 checks passed
@ndgrigorian ndgrigorian deleted the feature/dlpack-1-support branch May 13, 2024 04:40
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.

3 participants