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

Jump to bottom

usm_ndarray.to_device(dev, stream=queue) support #1331

Merged
merged 8 commits into from
Aug 11, 2023
Merged

usm_ndarray.to_device(dev, stream=queue) support #1331

merged 8 commits into from
Aug 11, 2023

Conversation

oleksandr-pavlyk
Copy link
Collaborator

This PR fixes two bugs: closes gh-1330, and fixes array_api_tests failure regarding usm_ndarray.to_device not supporting stream keyword.

  • 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?

@oleksandr-pavlyk
Support stream keyword in usm_ndarray.to_device per array API
f77f7a4
@oleksandr-pavlyk
Fix for gh-1330
c368034 
When source array must be broadcast, non-zero strides corresponding
to unit size dimensions must be zeroed out, otherwise if such dimension is
broadcasted, constructor would expected a bigger buffer than is really
necessary.
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

View rendered docs @ https://intelpython.github.io/dpctl/pulls/1331/index.html

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Array API standard conformance tests for dpctl=0.14.6dev0=py310h7bf5fec_121 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Array API standard conformance tests for dpctl=0.14.6dev0=py310h7bf5fec_122 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

@oleksandr-pavlyk oleksandr-pavlyk changed the title To device stream support usm_ndarray.to_device(dev, stream=queue) support Aug 9, 2023
Base automatically changed from enable-operators to master August 9, 2023 20:23
@oleksandr-pavlyk
Fixed bug introduced in 5c1a961
53315a9 
This commit short-circuited broadcastability of shapes for zero-size
rhs in __setitem__.

Refix array API test failure without introducing this regression and
reuse _manipulation_functions._broadcast_strides as suggested by
@ndgrigorian
@oleksandr-pavlyk
Added tests for examples fixed in prev. commit
8b03642
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the to_device-stream-support branch from b9f5fc7 to 8b03642 Compare August 9, 2023 21:33
@coveralls
Copy link
Collaborator

coveralls commented Aug 9, 2023 •
edited
Loading

Coverage Status

coverage: 84.921% (-0.002%) from 84.923% when pulling 706d80f on to_device-stream-support into ed93e02 on master.

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Array API standard conformance tests for dpctl=0.14.6dev1=py310h7bf5fec_35 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

@antonwolfy
Copy link
Collaborator

The following example is still not working:

In [1]: import dpnp, numpy, dpctl, dpctl.tensor as dpt

In [2]: a = dpt.ones((1, 2, 1, 4))

In [3]: b = dpt.empty((2, 3, 4))

In [4]: b[...] = a
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 b[...] = a

File dpctl/tensor/_usmarray.pyx:1178, in dpctl.tensor._usmarray.usm_ndarray.__setitem__()

File ~/miniconda3/envs/dpnp_dev/lib/python3.9/site-packages/dpctl/tensor/_copy_utils.py:307, in _copy_from_usm_ndarray_to_usm_ndarray(dst, src)
    305 else:
    306     src_same_shape = src
--> 307     src_same_shape.shape = common_shape
    309 _copy_same_shape(dst, src_same_shape)

File dpctl/tensor/_usmarray.pyx:576, in dpctl.tensor._usmarray.usm_ndarray.shape.__set__()

TypeError: Can not reshape array of size 8 into (2, 3, 4)

In [5]: dpctl.__version__
Out[5]: '0.14.6dev1+35.g5379d9342'

In [6]: conda list dpctl
# packages in environment at /home/xantvol/miniconda3/envs/dpnp_dev:
#
# Name                    Version                   Build  Channel
dpctl                     0.14.6dev1      py39h7bf5fec_35    file:///mnt/c/Users/antonvol/Downloads/dpctl Linux Python 3.9

@oleksandr-pavlyk
Fixed example for @antonwolfy, added tests based on it
346200e 
Also added comment to explain the logic of the code.
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.6dev1=py310h7bf5fec_39 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

@antonwolfy
Copy link
Collaborator

dpnp tests works fine with the PR, thank you @oleksandr-pavlyk !
I've filed a separate #1334 for remaining issues.

oleksandr-pavlyk added a commit that referenced this pull request Aug 11, 2023
@oleksandr-pavlyk
Fixes gh-1331
369eda4 
```
In [1]: import dpctl.tensor as dpt

In [2]: a = dpt.ones((2, 3))
   ...: dpt.reshape(a, (1, 6, 1)).flags
Out[2]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True

In [3]: a = dpt.ones((2, 3), order='F')
   ...: dpt.reshape(a, (1, 6, 1), order='F').flags
Out[3]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True

In [4]: a = dpt.ones((2, 3, 4))
   ...: dpt.sum(a, axis=(1, 2), keepdims=True).flags
Out[4]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True
```
@oleksandr-pavlyk
Fixes gh-1334
f5b96b7 
A non-empty array which is effectively 1D (only one dimension has
size greater than one) should be marked as both C- and F- contiguous.

```
In [1]: import dpctl.tensor as dpt

In [2]: a = dpt.ones((2, 3))
   ...: dpt.reshape(a, (1, 6, 1)).flags
Out[2]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True

In [3]: a = dpt.ones((2, 3), order='F')
   ...: dpt.reshape(a, (1, 6, 1), order='F').flags
Out[3]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True

In [4]: a = dpt.ones((2, 3, 4))
   ...: dpt.sum(a, axis=(1, 2), keepdims=True).flags
Out[4]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  WRITABLE : True
```
@oleksandr-pavlyk
Adds tests based on examples in issue gh-1334
706d80f
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.6dev1=py310h7bf5fec_44 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

@oleksandr-pavlyk oleksandr-pavlyk force-pushed the to_device-stream-support branch from ed4e003 to 706d80f Compare August 11, 2023 15:00
@antonwolfy
Copy link
Collaborator

I've checked the latest changes, they look good. The problem reported in #1334 is gone. No new issue is observed.
Thanks a lot @oleksandr-pavlyk !

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.6dev1=py310h7bf5fec_43 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

ndgrigorian
ndgrigorian approved these changes Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@ndgrigorian ndgrigorian left a comment

Choose a reason for hiding this comment

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

I've looked through the PR and don't see any glaring issues.

LGTM, thanks @oleksandr-pavlyk !

@oleksandr-pavlyk oleksandr-pavlyk merged commit 074ec3a into master Aug 11, 2023
@oleksandr-pavlyk oleksandr-pavlyk deleted the to_device-stream-support branch August 11, 2023 16:35
@github-actions
Copy link

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

@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request Aug 11, 2023
Merged
6 tasks
@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.6dev1=py310h7bf5fec_53 ran successfully.
Passed: 913
Failed: 87
Skipped: 119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndgrigorian ndgrigorian ndgrigorian approved these changes

@antonwolfy antonwolfy antonwolfy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Array assignment with broadcasting is not working
4 participants
@oleksandr-pavlyk @coveralls @antonwolfy @ndgrigorian