Move usm_ndarray into dpctl_ext.tensor#2807
Move usm_ndarray into dpctl_ext.tensor#2807vlad-perevezentsev wants to merge 272 commits intoinclude-dpctl-tensorfrom
usm_ndarray into dpctl_ext.tensor#2807Conversation
|
@antonwolfy @vlad-perevezentsev Maybe could we flatten to something like |
|
|
Based on warning report seems we still using |
| pybind11_add_module(${python_module_name} MODULE ${_module_src}) | ||
| add_sycl_to_target(TARGET ${python_module_name} SOURCES ${_module_src}) | ||
|
|
||
| target_link_libraries(${python_module_name} PRIVATE DpctlExtCAPI) |
There was a problem hiding this comment.
Why do we need that and how it worked previously without linking with dpctl lib?
There was a problem hiding this comment.
Previously dpnp4pybind11.hpp included dpctl_capi.h from the external dpctl package which was found via ${Dpctl_INCLUDE_DIRS} set by find_package(Dpctl).
Now dpnp4pybind11.hpp includes dpctl_ext_capi.h which is an internal header,and without linking to DpctlExtCAPI I got an error
dpnp4pybind11.hpp:41:10: fatal error: 'dpctl_ext_capi.h' file not found
41 | #include "dpctl_ext_capi.h"
There was a problem hiding this comment.
But the error is about include, not about linkage.
There was a problem hiding this comment.
This logic has been removed in #2830 so I do not see the point in fixing it here
In any case DpctlExtCAPI is an interface library that collects include paths and build dependencies , no actual linking takes place, just only handles build order and header locations
There was a problem hiding this comment.
The comment was about target_link_libraries usage, see docs:
Specify libraries or flags to use when linking a given target and/or its dependents.
Btw, I'm fine to resolve that in the follow-up PR.
| endif() | ||
|
|
||
| add_subdirectory(dpnp) | ||
| # DpctlExtCAPI: Interface library for dpctl_ext C-API |
There was a problem hiding this comment.
Is it a temporary approach, or do we need to define interface lib even when it will be exposed by dpctl when the migration is completed?
There was a problem hiding this comment.
It's a design decision either way: dpctl exposed PyUSMArrayObject for numba-dpex's sake. So this way, usm_ndarray would be exposed to C-API and could be interacted with from C code conveniently.
It could be removed, in which case, the api methods like i.e., cdef api char* UsmNDArray_GetData could also be removed (barring causing any problems in the dpctl_ext).
There was a problem hiding this comment.
I don't think we need to migrate the code, which will not be used by someone, I'd propose to drop that.
In any case we will able to port that in the future if someone will be interesting in.
There was a problem hiding this comment.
The PR removing С-API logic has been pushed in #2830
Please check if I have implemented your idea correctly
| @@ -0,0 +1,7 @@ | |||
| # DLPack header | |||
|
|
|||
| The header `dlpack.h` downloaded from `https://github.com/dmlc/dlpack.git` remote at tag v1.0rc commit [`62100c1`](https://github.com/dmlc/dlpack/commit/62100c123144ae7a80061f4220be2dbd3cbaefc7). | |||
There was a problem hiding this comment.
I believe we are using the newer tag than v1.0rc
There was a problem hiding this comment.
I have updated README.md and dlapck.h to v1.3
thank you!
There was a problem hiding this comment.
are you sure we support 1.3 version of DLPack?
| import math | ||
| import operator | ||
|
|
||
| import dpctl.tensor as dpt |
There was a problem hiding this comment.
We still have from dpctl.tensor interface in some dpnp comments
There was a problem hiding this comment.
All TODO comments have been removed in #2829
The remaining comments and docstrings containing dpctl.tensor will be updated in the following PR
| pybind11_add_module(${python_module_name} MODULE ${_module_src}) | ||
| add_sycl_to_target(TARGET ${python_module_name} SOURCES ${_module_src}) | ||
|
|
||
| target_link_libraries(${python_module_name} PRIVATE DpctlExtCAPI) |
There was a problem hiding this comment.
The comment was about target_link_libraries usage, see docs:
Specify libraries or flags to use when linking a given target and/or its dependents.
Btw, I'm fine to resolve that in the follow-up PR.
dpnp/dpnp_iface_statistics.py
Outdated
| # pylint: disable=no-name-in-module | ||
| import dpnp.backend.extensions.statistics._statistics_impl as statistics_ext | ||
| from dpctl_ext.tensor._numpy_helper import normalize_axis_index | ||
| from dpnp.dpnp_utils.dpnp_utils_common import ( |
There was a problem hiding this comment.
| from dpnp.dpnp_utils.dpnp_utils_common import ( | |
| from .dpnp_utils.dpnp_utils_common import ( |
There was a problem hiding this comment.
Updated
Thank you
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", DeprecationWarning) | ||
| from dpctl.tensor import __array_api_version__, DLDeviceType | ||
| from dpctl_ext.tensor import __array_api_version__, DLDeviceType |
There was a problem hiding this comment.
TODO comment is missing
There was a problem hiding this comment.
I have added it
Thank you !
This PR proposes to migrate the tensor interface (
usm_ndarray, dlpack, flags) intodpctl_ext/tensormakingdpnpindependent ofdpctl'stensor module.Updates: