Skip to content

Add a new SyclEventRaw class #520

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
Merged
merged 1 commit into from
Aug 11, 2021
Merged

Add a new SyclEventRaw class #520

merged 1 commit into from
Aug 11, 2021

Conversation

vlad-perevezentsev
Copy link
Collaborator

This PR will add a new SyclEventRaw class and tests for it.

@vlad-perevezentsev vlad-perevezentsev self-assigned this Aug 9, 2021
oleksandr-pavlyk
oleksandr-pavlyk suggested changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@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.

Minor changes needed, but looks great!

dpctl/_sycl_event.pyx Outdated


cdef class _SyclEventRaw:
""" Python wrapper class for a cl::sycl::event.
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
""" Python wrapper class for a cl::sycl::event.
""" Python wrapper class for a ``cl::sycl::event``.

dpctl/_sycl_event.pyx Outdated


cdef class SyclEventRaw(_SyclEventRaw):
""" Python wrapper class for a cl::sycl::event.
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
""" Python wrapper class for a cl::sycl::event.
""" Python wrapper class for a ``cl::sycl::event``.

dpctl/_sycl_event.pyx Outdated
cdef int ret = 0
if arg == None:
ret = self._init_event_default()
elif isinstance(arg, _SyclEventRaw):
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want here type(arg) is _SyclEventRaw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isinstance caters for inheritance while type equality checking does not. Don't we need to consider SyclEventRaw class when copying?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not do so for other classes, SyclDevice, SyclQueue, etc.

Perhaps we need to introduce .copy() methods to all of them.

dpctl/_sycl_event.pyx Outdated
)

cdef DPCTLSyclEventRef get_event_ref(self):
""" Returns the DPCTLSyclEventRef pointer for this class.
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
""" Returns the DPCTLSyclEventRef pointer for this class.
""" Returns the `DPCTLSyclEventRef` pointer for this class.

dpctl/_sycl_event.pyx Outdated
DPCTLEvent_Wait(self._event_ref)

def addressof_ref(self):
""" Returns the address of the C API DPCTLSyclEventRef pointer as
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
""" Returns the address of the C API DPCTLSyclEventRef pointer as
""" Returns the address of the C API `DPCTLSyclEventRef` pointer as

dpctl/_sycl_event.pyx Outdated
a size_t.

Returns:
The address of the DPCTLSyclEventRef object used to create this
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

dpctl/_sycl_event.pyx Outdated

Returns:
The address of the DPCTLSyclEventRef object used to create this
SyclEvent cast to a size_t.
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
SyclEvent cast to a size_t.
`SyclEvent` cast to a size_t.

dpctl/_sycl_event.pyx Outdated
The address of the DPCTLSyclEventRef object used to create this
SyclEvent cast to a size_t.
"""
return int(<size_t>self._event_ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

Call to int constructor is not needed here.

@coveralls
Copy link
Collaborator

coveralls commented Aug 10, 2021 •
edited
Loading

Coverage Status

Coverage increased (+1.3%) to 66.311% when pulling bb81d90 on vlad-perevezentsev:cython_SyclEvent into e5c50ea on IntelPython:master.

@oleksandr-pavlyk
Copy link
Contributor

@vlad-perevezentsev The Windows Py3.7 build failed because there is not OpenCL CPU driver installed on that machine:

[05:13:44] :	 [Step 2/2] ..\_test_env\lib\site-packages\dpctl\tests\test_sycl_event.py:41: 
[05:13:44] :	 [Step 2/2] _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[05:13:44] :	 [Step 2/2] >   ???
[05:13:44] :	 [Step 2/2] E   dpctl._sycl_queue.SyclQueueCreationError: SYCL Device 'opencl:cpu' could not be created.
[05:13:44] :	 [Step 2/2] dpctl\_sycl_queue.pyx:327: SyclQueueCreationError
[05:13:44] :	 [Step 2/2] ---------------------------- Captured stderr call -----------------------------
[05:13:44] :	 [Step 2/2] Could not find a device that matches the specified filter(s)! -1 (CL_DEVICE_NOT_FOUND)

Could you please add logic to skip such a test if the queue creation did not succeed? Please look at other tests (in test file for queue submit) for examples.

@oleksandr-pavlyk oleksandr-pavlyk changed the base branch from master to refactor/SyclEvent August 11, 2021 15:27
@oleksandr-pavlyk oleksandr-pavlyk merged commit 414de9c into IntelPython:refactor/SyclEvent Aug 11, 2021
vlad-perevezentsev added a commit to vlad-perevezentsev/dpctl that referenced this pull request Aug 23, 2021
@vlad-perevezentsev
Add a new SyclEventRaw class (IntelPython#520)
bcac0c4
vlad-perevezentsev added a commit that referenced this pull request Aug 23, 2021
@vlad-perevezentsev
Add a new SyclEventRaw class (#520)
7b2c209
oleksandr-pavlyk pushed a commit that referenced this pull request Aug 25, 2021
@vlad-perevezentsev @oleksandr-pavlyk
Add a new SyclEventRaw class (#520)
729cbed
@vlad-perevezentsev vlad-perevezentsev deleted the cython_SyclEvent branch June 20, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleksandr-pavlyk oleksandr-pavlyk oleksandr-pavlyk approved these changes

Assignees

@vlad-perevezentsev vlad-perevezentsev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vlad-perevezentsev @coveralls @oleksandr-pavlyk