Skip to content

Add a new SyclEventRaw class#520

Merged
oleksandr-pavlyk merged 1 commit intoIntelPython:refactor/SyclEventfrom
vlad-perevezentsev:cython_SyclEvent
Aug 11, 2021
Merged

Add a new SyclEventRaw class#520
oleksandr-pavlyk merged 1 commit intoIntelPython:refactor/SyclEventfrom
vlad-perevezentsev:cython_SyclEvent

Conversation

@vlad-perevezentsev
Copy link
Copy Markdown
Collaborator

@vlad-perevezentsev vlad-perevezentsev commented Aug 9, 2021

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
Copy Markdown
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!

Comment thread dpctl/_sycl_event.pyx Outdated


cdef class _SyclEventRaw:
""" Python wrapper class for a cl::sycl::event.
Copy link
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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

Comment thread dpctl/_sycl_event.pyx Outdated


cdef class SyclEventRaw(_SyclEventRaw):
""" Python wrapper class for a cl::sycl::event.
Copy link
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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

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

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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
Copy Markdown
Collaborator Author

@vlad-perevezentsev vlad-perevezentsev Aug 10, 2021

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
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 10, 2021

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.

Comment thread dpctl/_sycl_event.pyx Outdated
)

cdef DPCTLSyclEventRef get_event_ref(self):
""" Returns the DPCTLSyclEventRef pointer for this class.
Copy link
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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.

Comment thread 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
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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

Comment thread dpctl/_sycl_event.pyx Outdated
a size_t.

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

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

Choose a reason for hiding this comment

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

Same here

Comment thread dpctl/_sycl_event.pyx Outdated

Returns:
The address of the DPCTLSyclEventRef object used to create this
SyclEvent cast to a size_t.
Copy link
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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.

Comment thread 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
Copy Markdown
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Aug 9, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

oleksandr-pavlyk commented Aug 10, 2021

@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

1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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