Skip to content
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

[enhancement] remove contiguous check from _check_array #2185

Merged
merged 24 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions onedal/datatypes/_data_conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ def _convert_one_to_table(arg):
def to_table(*args):
"""Create oneDAL tables from scalars and/or arrays.

Note: this implementation can be used with contiguous scipy.sparse, numpy
ndarrays, DPCTL/DPNP usm_ndarrays and scalars. Tables will use pointers to the
original array data. Scalars will be copies. Arrays may be modified in-
place by oneDAL during computation. This works for data located on CPU and
SYCL-enabled Intel GPUs. Each array may only be of a single datatype (i.e.
each must be homogeneous).
Note: this implementation can be used with scipy.sparse, numpy ndarrays,
DPCTL/DPNP usm_ndarrays and scalars. Tables will use pointers to the
original array data. Scalars and non-contiguous arrays will be copies.
Arrays may be modified in-place by oneDAL during computation. This works
for data located on CPU and SYCL-enabled Intel GPUs. Each array may only
be of a single datatype (i.e. each must be homogeneous).

Parameters
----------
Expand Down
22 changes: 12 additions & 10 deletions onedal/datatypes/data_conversion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,18 +155,20 @@ dal::table convert_to_table(PyObject *obj) {
}
if (is_array(obj)) {
PyArrayObject *ary = reinterpret_cast<PyArrayObject *>(obj);
if (array_is_behaved_C(ary) || array_is_behaved_F(ary)) {
if (!PyArray_ISCARRAY_RO(ary) && !PyArray_ISFARRAY_RO(ary)) {
// NOTE: this will make a C-contiguous deep copy of the data
// this is expected to be a special case
ary = PyArray_GETCONTIGUOUS(ary);
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, could you explain what would happen if an error occurs here? Additionally, is there a risk of a memory leak in this scenario?
For example, is there some error flag that should be checked?
Wouldn't it be safer to use the Python NumPy API for such checks and conversions?

Copy link
Contributor Author

@icfaust icfaust Nov 25, 2024

Choose a reason for hiding this comment

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

The Decref is necessary to prevent a memory leak (otherwise leaks in BasicStatistics occur in all CIs), the memory is effectively owned by the table object, who will call its destructor with the pycapsule. This work makes to_table support non-contiguous arrays, which will simplify the python coding occurring before to_table. This work will enable _check_array to be moved from onedal to sklearnex simplifying the rollout of the new finite checker to SVM and neighbors algorithms. There is no error handling available (that I can find) in the numpy C-api as also numpy generally doesn't raise errors like that. The checks that it is 1) a numpy array and 2) of certain aligned and type characteristics will prevent errors, and the use of the numpy c-api is very standardized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh thank you for doing that research @david-cortes-intel , that would mean in the case of a failure in allocation, it will fail to decref. I will add a check there.

res = convert_to_table(reinterpret_cast<PyObject *>(ary));
Py_DECREF(ary);
return res;
}
#define MAKE_HOMOGEN_TABLE(CType) res = convert_to_homogen_impl<CType>(ary);
SET_NPY_FEATURE(array_type(ary),
array_type_sizeof(ary),
MAKE_HOMOGEN_TABLE,
throw std::invalid_argument("Found unsupported array type"));
SET_NPY_FEATURE(array_type(ary),
array_type_sizeof(ary),
MAKE_HOMOGEN_TABLE,
throw std::invalid_argument("Found unsupported array type"));
#undef MAKE_HOMOGEN_TABLE
}
else {
throw std::invalid_argument(
"[convert_to_table] Numpy input Could not convert Python object to onedal table.");
}
}
else if (strcmp(Py_TYPE(obj)->tp_name, "csr_matrix") == 0 || strcmp(Py_TYPE(obj)->tp_name, "csr_array") == 0) {
PyObject *py_data = PyObject_GetAttrString(obj, "data");
Expand Down
24 changes: 22 additions & 2 deletions onedal/datatypes/data_conversion_sua_iface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

namespace oneapi::dal::python {

using namespace pybind11::literals;
// Please follow <https://intelpython.github.io/dpctl/latest/
// api_reference/dpctl/sycl_usm_array_interface.html#sycl-usm-array-interface-attribute>
// for the description of `__sycl_usm_array_interface__` protocol.
Expand All @@ -42,6 +43,8 @@ namespace oneapi::dal::python {
// of `__sycl_usm_array_interface__` protocol.
template <typename Type>
dal::table convert_to_homogen_impl(py::object obj) {
dal::table res{};

// Get `__sycl_usm_array_interface__` dictionary representing USM allocations.
auto sua_iface_dict = get_sua_interface(obj);

Expand All @@ -64,6 +67,25 @@ dal::table convert_to_homogen_impl(py::object obj) {
// Get oneDAL Homogen DataLayout enumeration from input object shape and strides.
const auto layout = get_sua_iface_layout(sua_iface_dict, r_count, c_count);

if (layout == dal::data_layout::unknown){
// NOTE: this will make a C-contiguous deep copy of the data
// if possible, this is expected to be a special case
py::object copy;
if (py::hasattr(obj, "copy")){
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it guaranteed that the copy will be C-contiguous? It will always be the case with numpy, but what about other packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of now only dpctl and dpnp use this standard:
https://github.com/IntelPython/dpnp/blob/master/dpnp/dpnp_container.py#L137 will specify "K" to dpctl.tensor's copy: https://github.com/IntelPython/dpctl/blob/master/dpctl/tensor/_copy_utils.py#L574 which if its not F aligned (which is the case) it will default to C alignment. asarray in dpctl https://github.com/IntelPython/dpctl/blob/master/dpctl/tensor/_ctors.py#L483 also has "K" as default. We test this circumstance in the test that is modified in this PR for dpnp and dpctl. If a new sycl_usm_namespace array type comes out, we can come back to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about other libraries that implement the array protocol? XArray also has "copy" for example:
https://docs.xarray.dev/en/latest/generated/xarray.DataArray.copy.html

copy = obj.attr("copy")();
}
else if (py::hasattr(obj, "__array_namespace__")){
const auto space = obj.attr("__array_namespace__")();
copy = space.attr("asarray")(obj, "copy"_a = true);
}
else {
throw std::runtime_error("Wrong strides");
}
res = convert_to_homogen_impl<Type>(copy);
copy.dec_ref();
return res;
}

// Get `__sycl_usm_array_interface__['data'][0]`, the first element of data entry,
// which is a Python integer encoding USM pointer value.
const auto* const ptr = reinterpret_cast<const Type*>(get_sua_ptr(sua_iface_dict));
Expand All @@ -79,8 +101,6 @@ dal::table convert_to_homogen_impl(py::object obj) {
// Use read-only accessor for onedal table.
bool is_readonly = is_sua_readonly(sua_iface_dict);

dal::table res{};

if (is_readonly) {
res = dal::homogen_table(queue,
ptr,
Expand Down
15 changes: 3 additions & 12 deletions onedal/datatypes/tests/test_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -377,23 +377,14 @@ def test_sua_iface_interop_unsupported_dtypes(dataframe, queue, dtype):
def test_to_table_non_contiguous_input(dataframe, queue):
if dataframe in "dpnp,dpctl" and not _is_dpc_backend:
pytest.skip("__sycl_usm_array_interface__ support requires DPC backend.")
X = np.mgrid[:10, :10]
X, _ = np.mgrid[:10, :10]
X = _convert_to_dataframe(X, sycl_queue=queue, target_df=dataframe)
X = X[:, :3]
sua_iface, _, _ = _get_sycl_namespace(X)
# X expected to be non-contiguous.
assert not X.flags.c_contiguous and not X.flags.f_contiguous

# TODO:
# consistent error message.
if dataframe in "dpnp,dpctl":
expected_err_msg = (
"Unable to convert from SUA interface: only 1D & 2D tensors are allowed"
)
else:
expected_err_msg = "Numpy input Could not convert Python object to onedal table."
with pytest.raises(ValueError, match=expected_err_msg):
to_table(X)
X_t = to_table(X)
assert X_t and X_t.shape == (10, 3) and X_t.has_data


@pytest.mark.skipif(
Expand Down
2 changes: 1 addition & 1 deletion onedal/datatypes/utils/sua_iface_helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ dal::data_layout get_sua_iface_layout(const py::dict& sua_dict,
return dal::data_layout::column_major;
}
else {
throw std::runtime_error("Wrong strides");
return dal::data_layout::unknown;
}
}
else {
Expand Down
9 changes: 0 additions & 9 deletions onedal/utils/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,6 @@ def _check_array(

if sp.issparse(array):
return array

# TODO: Convert this kind of arrays to a table like in daal4py
if not array.flags.aligned and not array.flags.writeable:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From numpy documentation: https://numpy.org/devdocs/dev/alignment.html only structured arrays will cause this, meaning that this is a non issue based on the dtypes of use for oneDAL (float32, float64, int32, and int64), and would failt to convert in table.cpp anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think numpy also allows creating non-aligned arrays out of custom non-owned pointers, for example through PyArray_SimpleNewFromData, so in theory there could be a non-aligned float32 array or similar. But it'd be a very unlikely input, since default allocators in most platforms are aligned.

Also an easier conversion could be through np.require.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll probably because of this too I will make a stricter check on C++ side https://github.com/intel/scikit-learn-intelex/blob/main/onedal/decomposition/pca.py#L150 probably using https://numpy.org/devdocs/reference/c-api/array.html#c.PyArray_FromArray where I can specify the requirements to be aligned and owned (rather than use PyArray_GETCONTIGUOUS)

Copy link
Contributor Author

@icfaust icfaust Nov 25, 2024

Choose a reason for hiding this comment

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

@david-cortes-intel I looked into it, the checks PyArray_ISFARRAY_RO and PyArray_ISCARRAY_RO check for alignment natively (https://numpy.org/devdocs/reference/c-api/array.html#c.PyArray_ISFARRAY_RO), and then PyArray_GETCONTIGUOUS also returns an aligned (well-behaved) copy regardless of the ownership. The change in the checks was to make sure that numpy objects wouldn't cause an infinite recursion. So the issue was taken care of, even if I didn't realize it. Just as a note, I am trying to minimize the number of numpy calls because of upcoming array_api support which we cannot rely on direct numpy calls on non-numpy arrays (say dpctl tensors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With respect to non-ownership, I don't want to step on anyone's toes, as changes to PCA are occuring here which may inpact the ownership check there: #2106

array = np.array(array.tolist())

# TODO: If data is not contiguous copy to contiguous
# Need implemeted numpy table in oneDAL
if not array.flags.c_contiguous and not array.flags.f_contiguous:
array = np.ascontiguousarray(array, array.dtype)
return array


Expand Down
Loading