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/dpctl memcpy async #529

Merged

Conversation

reazulhoque
Copy link
Contributor

This updates API changes in dpctl for memcpy. IntelPython/dpctl#557

@@ -19,13 +19,13 @@ requirements:
- setuptools
- cython
- numba 0.54*
- dpctl >=0.9*
- dpctl >=0.9.1.dev*
Copy link
Contributor

Choose a reason for hiding this comment

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

This will presently make it break, since no such package is yet available on the channels.

Copy link
Contributor

@PokhodenkoSA PokhodenkoSA Sep 1, 2021

Choose a reason for hiding this comment

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

@oleksandr-pavlyk
If API changed then dpctl version should be 0.10.0 (dev0).
0.9.1 is a patch version.
Right?

Copy link
Contributor

Choose a reason for hiding this comment

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

To play nice with multi-versioning on Linux - yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- dpctl >=0.9.1.dev*
- dpctl >=0.10.0*

@@ -31,15 +31,21 @@ def common_impl(a, b, out, dpnp_func, PRINT_DEBUG):
sycl_queue = dpctl_functions.get_current_queue()

b_usm = dpctl_functions.malloc_shared(b.size * b.itemsize, sycl_queue)
dpctl_functions.queue_memcpy(sycl_queue, b_usm, b.ctypes, b.size * b.itemsize)
event = dpctl_functions.queue_memcpy(
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Sep 1, 2021

Choose a reason for hiding this comment

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

Wouldn't it be better to create a function:

def synchronous_memcpy(q, dst, src, size):
     event = dpctl_functions.queue_memcpy(q, dst, src, size)
     dpctl_functions.event_wait(event)
     dpctl_functions.event_delete(event)

and use it throughout, perhaps even in dpctl_functions module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Opinions?

Copy link
Contributor

@PokhodenkoSA PokhodenkoSA Sep 5, 2021

Choose a reason for hiding this comment

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

About dpctl API I would like to propose to try to avoid breaking changes:

def queue_memcpy(..., wait=True): 
    ...
    if (wait):
        dpctl_functions.event_wait(event)
        dpctl_functions.event_delete(event)
        return None
    return event

def queue_memcpy_async(...):
    return queue_memcpy(..., wait=False)

- dpnp >=0.7* # [linux]
- wheel
run:
- python
- numba 0.54*
- dpctl >=0.9*
- dpctl >=0.9.1.dev*
Copy link
Contributor

Choose a reason for hiding this comment

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

@reazulhoque It is not necessary to use dev here. Just >=0.9.1*

Suggested change
- dpctl >=0.9.1.dev*
- dpctl >=0.9.1*

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- dpctl >=0.9.1.dev*
- dpctl >=0.10.0*

@PokhodenkoSA
Copy link
Contributor

PokhodenkoSA commented Sep 2, 2021

Waiting for dpctl 0.10.0dev0 upload to channels.

Copy link
Contributor

@PokhodenkoSA PokhodenkoSA left a comment

Choose a reason for hiding this comment

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

I think we can merge it.

@PokhodenkoSA PokhodenkoSA merged commit 16d72c9 into IntelPython:main Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dpctl Integration with dpctl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants