[enhancement] add dlpack support to to_table
#2275
Conversation
| MAKE_QUEUED_HOMOGEN(ptr); | ||
| } | ||
| else { | ||
| auto* const mut_ptr = const_cast<T*>(ptr); |
There was a problem hiding this comment.
| auto* const mut_ptr = const_cast<T*>(ptr); | |
| auto* mut_ptr = const_cast<T*>(ptr); |
Same thought about making the pointer const as above.
By the way, the data is provided non-const, you are making it const by choice above and const-casting it away again here.
At first glance, the non-const usage is dominant and I don't see a point in ever making the data const. Did I miss something?
There was a problem hiding this comment.
ahh, I lifted this part from here, maybe we need to modify it there too? https://github.com/uxlfoundation/scikit-learn-intelex/blob/main/onedal/datatypes/sycl_usm/data_conversion.cpp#L113
There was a problem hiding this comment.
Just as an update: with the discussion of the readonly, I removed the aspect entirely.
|
/intelci: run |
| } | ||
|
|
||
| #define MAKE_HOMOGEN_TABLE(CType) \ | ||
| res = versioned ? convert_to_homogen_impl<CType, DLManagedTensorVersioned>(dlmv, q_obj) \ |
There was a problem hiding this comment.
Since you are assigning value to the already defined object I would suggest to use if statement instead of ternary
There was a problem hiding this comment.
Yeah, I was trying to keep it small/ for readability, I know it may cause a slight difference in the output assembly.
| : convert_to_homogen_impl<CType, DLManagedTensor>(dlm, q_obj); | ||
| SET_CTYPE_FROM_DAL_TYPE(dtype, | ||
| MAKE_HOMOGEN_TABLE, | ||
| throw std::invalid_argument("Found unsupported array type")); |
There was a problem hiding this comment.
Shouldn't an error message be declared in the separate file alongside other error messages?
There was a problem hiding this comment.
Follows convention set out in the original numpy interfaces (https://github.com/uxlfoundation/scikit-learn-intelex/blob/main/onedal/datatypes/numpy/data_conversion.cpp#L205). I'm not saying its right, but its consistent.
| std::int32_t get_ndim(const DLTensor& tensor) { | ||
| // check if 1 or 2 dimensional, and return the number of dimensions | ||
| const std::int32_t ndim = tensor.ndim; | ||
| if (ndim != 2 && ndim != 1) { |
There was a problem hiding this comment.
What about the zero dimensional tensors (scalars)?
There was a problem hiding this comment.
Will make a test explicitly for testing this scenario and handling it gracefully
| @@ -35,6 +35,7 @@ | |||
| get_dataframes_and_queues, | |||
| ) | |||
| from onedal.tests.utils._device_selection import get_queues, is_dpctl_device_available | |||
| from onedal.utils._array_api import _get_sycl_namespace | |||
There was a problem hiding this comment.
FYI, it's very uncommon in Python (and ML) world to name importance entities starting with underscore. Some Python interpreters and widely used libraries do not support this naming. Famously you can not use any _* entities in torch.jit context due to the inability to inspect these functions
There was a problem hiding this comment.
Yeah, I totally agree. Private functions should be private. Will follow up with changes to make this (wasn't my doing, but I guess I did allow it to happen).
There was a problem hiding this comment.
+1, but I do want to add that the rules are more relaxed in testing context.
Co-authored-by: Alexander Andreev <[email protected]>
Description
This PR introduces
__dlpack__tensor (https://github.com/dmlc/dlpack) consumption byto_tableallowing for zero-copy use of data in oneDAL. This is important for enabling array_api support and is a pre-requisite for #2096 (array api dispatching). That PR is then a pre-requisite for #2100 #2106 #2189 #2206 #2207 and #2209. Sklearn provides array_api support for some algorithms. If we wish to fully support zero copy of sycl_usm inputs, we need to be able to consume array_api inputs due to underlying sklearn dependencies (validate_data,check_array, etc.). While we support Sycl usm ndarrays (dpctl,dpnp) via the__sycl_usm_array_interface__method in the onedal folder estimators, to properly interface estimators in the sklearnex folder, we need to support the__dlpack__method of arrays/tensors. This PR does that and greatly simplifies the necessary logic in #2096 and the follow-up PRs. This PR also provides the added benefit of working with other frameworks which support SYCL gpu data which have__dlpack__interfaces (i.e. PyTorch).NOTES:
This PR continues from [enhancement] Refactor
onedal/datatypesin preparation for dlpack support #2195 to integrate dlpack support, and it takes code from DataManagement update #1568. Please use DataManagement update #1568 as reference, though it contained some mistakes.This new functionality is not yet exposed publicly and therefore must be added to the documentation with ENH: Array API dispatching #2096.
This aspect is not yet benchmarkable/ has nothing to benchmark against.
Memory leak checking using the infrastructure from ENH: Data management update to support SUA ifaces for Homogen OneDAL tables #2045 (only CPU at the moment, will be modified if/when PyTorch support is brought online)
It does numeric testing via onedal's
assert_all_finitetesting usingarray_api_strictarrays, as that has a simple interface to the backend with no checking of array aspects on the python-side beforeto_table.A special testing class is created to verify for SYCL device support and is used in
test_data.py.This does not have any work with returning
__dlpack__supported arrays as results. And must be done as a follow-up PR (probably also a good idea in order to ease reviewing). Therefore, this is only about array/tensor consumption.TODO: add a onedal function which checks a dlpack tensor for C-contiguity or F-contiguity similar to the
flagsattribute of numpy/dpctl/dpnp. This is out of the scope of this PR, but is necessary for assert_all_finite support for the next step in array_api work.PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance