Skip to content

Dpctl c api constructors added, dpctl/include restructured #685

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

Merged
merged 9 commits into from
Dec 2, 2021

Conversation

oleksandr-pavlyk
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk commented Nov 23, 2021

Refactored dpctl/include layout. Instead of containing all files from libsyclinterface/include it introduces substructure:

   dpctl/
      include/
         syclinterface/
              *.h   # all the files that used to be in dpctl/include
         syclinterface.h   
         dpctl_capi.h
         dpctl4pybind11.hpp

The newly added files are residing in new folder dpctl/apis/include from which build_backend.py copies then into dpctl/include

Modified _backend.pxd to include "syclinterface/" in the path.

Modified build_backend.py to reassemble dpctl/include from install/include and dpctl/apis/include.

Added make_Sycl* C-API functions to create SyclDevice, SyclQueue, SyclContext and SyclEvent from corresponding opaque reference. Used get_device_ref and make_SyclDevice to construct pybind11 type caster directly mapping dpctl.SyclDevice Python object to sycl::device in pybind11.

Modified examples files to use new headers, and use casters.

Verified that all Cython extensions and Pybind11 extensions are building and are running as expected.

@github-actions
Copy link

@coveralls
Copy link
Collaborator

coveralls commented Nov 23, 2021

Coverage Status

Coverage increased (+0.06%) to 81.192% when pulling d42e403 on dpctl_c_api-constructors into 6fb23a3 on master.

@oleksandr-pavlyk oleksandr-pavlyk changed the title Dpctl c api constructors Dpctl c api constructors added, dpctl/include restructured Nov 23, 2021
@oleksandr-pavlyk
Copy link
Contributor Author

@densmirn @Alexander-Makaryev Most likely dpnp is going to be affected by this change. Once this PR is merged, the downstream (relative to dpctl) libraries will need to #include "dpctl_sycl_interface.h" instead of including individual headers, such as dpctl_sycl_types.h and dpctl_sycl_queue_manager.h.

@napetrov Please forward this to relevant people working on scikit-learn-intelex.

@oleksandr-pavlyk
Copy link
Contributor Author

@diptorupd This change is going to cause some disruption in downstream code:

  1. https://github.com/IntelPython/numba-dppy/blob/main/numba_dppy/dpctl_iface/usm_shared_allocator_ext.c#L30
  2. https://github.com/IntelPython/dpnp/blob/master/dpnp/backend/src/queue_sycl.hpp#L44
  3. https://github.com/intel/scikit-learn-intelex/blob/master/src/dpctl_interop/daal_context_service.cpp#L20

I am not sure what DPPLQueueMgr_GetCurrentQueue is in scikit-learn-intelex, and I did not see it defined anywhere in the project. Perhaps @napetrov could clarify.

But perhaps to minimize the disruption, we could temporarily add symbolic links in dpctl/include/ for some commonly used header files?

@diptorupd
Copy link
Contributor

But perhaps to minimize the disruption, we could temporarily add symbolic links in dpctl/include/ for some commonly used header files?

No please. It is a minimal change that we have enough time to fix in downstream. I will suggest that we keep the change out of 0.12 and then fix it.

@oleksandr-pavlyk
Copy link
Contributor Author

oleksandr-pavlyk commented Nov 24, 2021

I will suggest that we keep the change out of 0.12 and then fix it.

I am afraid I need parts of this PR (support for dpctl4pybind11) in 0.12.

How about we add a header file (`dpctl/include/dpctl_sycl_queue_manager.h` that contains `#error` with the text explaining what to do.

This header is to be removed before 0.12 tag is created.

Update

Downstream projects can use __has_include("name.h") preprocessor check:

#if defined __has_include
#   if __has_include("dpctl_sycl_interface.h")
#       include "dpctl_sycl_interface.h"
#   else
#       include "dpctl_sycl_queue_manager.h"    // include one or more explicit dpctl_sycl* headers
#   endif
#else
#   include "dpctl_sycl_interface.h"
#endif

for a transition period.

I would therefore merge this to master once this change is added to downstream projects.

oleksandr-pavlyk added a commit to IntelPython/dpnp that referenced this pull request Nov 25, 2021
Use __has_include to minimize disruption that the said reorg may cause
oleksandr-pavlyk added a commit to IntelPython/numba-dpex that referenced this pull request Nov 25, 2021
Use `__has_include` to work with both layouts
Alexander-Makaryev added a commit to IntelPython/dpnp that referenced this pull request Nov 29, 2021
Use __has_include to minimize disruption that the said reorg may cause

Co-authored-by: Alexander-Makaryev <alexander.makaryev@gmail.com>
@oleksandr-pavlyk
Copy link
Contributor Author

I would therefore merge this to master once this change is added to downstream projects.

Actually, testing locally, there are no conflicts between this branch and the work in #683

This is needed in order to be able to write pybind11
casters directly mapping sycl classes to `dpctl.Sycl*` classes
for SyclContext, SyclQueue, SyclDevice and SyclEvent classes.
1. `dpctl/include` folder that is being installed in the site-packages/dpctl
   changes layout. It now has the structure
    dpctl/
      include/
        syclinterface/
	  *.h  # all files that used to be in dpctl/include are moving here
        syclinterface.h
        dpctl_capi.h
        dpctl4pybind11.hpp

_backend.pxd was modified to include syclinterface/ into path of header files
from which we are importing
build_backend.py was modified to copy install/include into
dpctl/include/syclinterface instead of dpctl/include, and steps
were added to also copy newly added dpctl/apis/include/* files into
dpctl/include/

2. Now, to work with syclinterface, one only needs
    `#include "syclinterface.h"`

   assuming `-I$(python -c "import dpctl; print(dpctl.get_include())")`
   is used

3. To work with `dpctl` objects using Python C-API one needs
   ``#include "dpctl_capi.h"`` where objects, types and C-API
   functions are declared.

4. To work with pybind11, one needs ``#include "dpctl4pybind11.hpp"``
   where type casters are defined to map betwen ``sycl::queue`` and
   `dpctl.SyclQueue`, and correspondingly for ``sycl::device``,
   ``sycl::context`` and ``sycl::event``.

Modifications to examples are forthcoming

_backend.pxd should use syclinterface/ to reference .h files
Fixed runtime_error verbiage.
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the dpctl_c_api-constructors branch from 60c5523 to d42e403 Compare December 1, 2021 23:46
@oleksandr-pavlyk oleksandr-pavlyk merged commit bbde0f5 into master Dec 2, 2021
@oleksandr-pavlyk oleksandr-pavlyk deleted the dpctl_c_api-constructors branch December 2, 2021 04:14
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

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

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.

4 participants