Skip to content

isolate dpnp sua offset fix - #3330

Merged
ethanglaser merged 4 commits into
uxlfoundation:mainfrom
ethanglaser:dev/eglaser-dpnp-sua-offset-fix
Jul 22, 2026
Merged

isolate dpnp sua offset fix#3330
ethanglaser merged 4 commits into
uxlfoundation:mainfrom
ethanglaser:dev/eglaser-dpnp-sua-offset-fix

Conversation

@ethanglaser

@ethanglaser ethanglaser commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Isolated from #3322 where it was initially discovered. Basically any dpnp non-host offloaded array slicing operation would unsuccessfully apply an offset (ie X_dpnp_device[5:10] would actually pull X_dpnp_device[0:5]). Per Claude:

Root cause

The C++ convert_to_homogen_impl in onedal/datatypes/sycl_usm/data_conversion.cpp built the oneDAL table from __sycl_usm_array_interface__['data'][0] (the base allocation pointer) but never applied the offset field. A dpnp slice view (e.g. X[a:b]) keeps the same data[0] base pointer and encodes its start position via offset (in elements, not bytes). Without applying it, oneDAL read from row 0 of the allocation for every view rather than from the view's actual start.

Symptom

Under array_api_dispatch=True, IncrementalEmpiricalCovariance.fit(dpnp_X) produced a wrong location_/score — surfaced as test_whitened_toy_score[dpnp-*] returning ≈ -13.58 vs expected ≈ -14.18 (identical on CPU and GPU). IncrementalEmpiricalCovariance.fit batches its input via slice views (X[batch_start:batch_end]), so every batch after the first read the wrong rows. Single-batch fits (batch_size ≥ n_samples) and explicitly copied batches were unaffected.

Minimal repro, independent of covariance: a dpnp X[5:10] view round-tripped through onedal.datatypes.to_table/from_table returns row-0 data instead of row-5.

Why it wasn't caught before

On main, dpnp inputs without array_api_dispatch are routed through _transfer_to_host (→ asnumpy) before reaching the SUA→table path, so slice views never hit this conversion. This is a latent backend bug; #3322 (removing non-array-API dpnp support) only exposes it by passing dpnp slice views directly to to_table.

Fix

Added get_sua_offset() in sycl_usm_utils.cpp/.hpp (reads the optional offset, defaults to 0) and applied it: ptr = get_sua_ptr(...) + offset.


Checklist:

Completeness and readability

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least a summary table with measured data, if performance change is expected.
  • I have provided justification why performance and/or quality metrics have changed or why changes are not expected.
  • I have extended the benchmarking suite and provided a corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@ethanglaser

Copy link
Copy Markdown
Contributor Author

/intelci: run

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
onedal/datatypes/sycl_usm/data_conversion.cpp 0.00% 0 Missing and 2 partials ⚠️
onedal/datatypes/sycl_usm/sycl_usm_utils.cpp 50.00% 0 Missing and 1 partial ⚠️
Flag Coverage Δ
azure 82.09% <ø> (ø)
github 74.45% <25.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
onedal/datatypes/sycl_usm/sycl_usm_utils.cpp 52.17% <50.00%> (+2.84%) ⬆️
onedal/datatypes/sycl_usm/data_conversion.cpp 41.79% <0.00%> (+0.45%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@icfaust icfaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ethanglaser quick question, wouldn't this have thrown off all use_raw_input results?

@ethanglaser

Copy link
Copy Markdown
Contributor Author

@ethanglaser quick question, wouldn't this have thrown off all use_raw_input results?

Yes. The reason it was not caught is there are minimal slicing operations in the spmd test suite. But any slicing operation in these device-only dpnp arrays would not account for the indicated offset.

@ethanglaser

Copy link
Copy Markdown
Contributor Author

Pretty big bug and equally large test gap

@ethanglaser
ethanglaser marked this pull request as ready for review July 20, 2026 22:05
Copilot AI review requested due to automatic review settings July 20, 2026 22:05
@ethanglaser
ethanglaser requested a review from Vika-F as a code owner July 20, 2026 22:05
@ethanglaser

Copy link
Copy Markdown
Contributor Author

/intelci: run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a latent bug in the DPC (SYCL USM) zero-copy conversion path where dpnp slice views were converted to oneDAL tables without applying the __sycl_usm_array_interface__['offset'], causing oneDAL to read from the wrong starting element for views.

Changes:

  • Add get_sua_offset() helper to read the optional SUA offset field (defaulting to 0).
  • Apply the SUA offset during convert_to_homogen_impl pointer construction so slice views point at the correct starting element.
  • Add a regression test covering dpnp slice-view roundtrips through to_table / from_table.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
onedal/datatypes/tests/test_data.py Adds regression test for dpnp slice-view offset handling in SUA-backed table conversions.
onedal/datatypes/sycl_usm/sycl_usm_utils.hpp Declares new get_sua_offset() helper in the SUA utilities API.
onedal/datatypes/sycl_usm/sycl_usm_utils.cpp Implements get_sua_offset() to retrieve the optional SUA offset field.
onedal/datatypes/sycl_usm/data_conversion.cpp Applies SUA offset when building the oneDAL homogen table pointer from SUA metadata.

Comment on lines +96 to +101
std::int64_t get_sua_offset(const py::dict& sua) {
if (!sua.contains("offset") || sua["offset"].is_none()) {
return 0l;
}
return sua["offset"].cast<std::int64_t>();
}
Comment thread onedal/datatypes/tests/test_data.py Outdated
Comment on lines +252 to +254
X_view = X_dp[start:]
X_roundtrip = from_table(to_table(X_view), like=X_dp)

@ethanglaser

Copy link
Copy Markdown
Contributor Author

/intelci: run

Comment thread onedal/datatypes/tests/test_data.py Outdated
@ethanglaser

Copy link
Copy Markdown
Contributor Author

/intelci: run

@ethanglaser
ethanglaser merged commit 0f39f6e into uxlfoundation:main Jul 22, 2026
30 of 31 checks passed
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