Skip to content

[CI] upgrade dpctl/dpnp versions#2414

Merged
icfaust merged 71 commits into
uxlfoundation:mainfrom
icfaust:dev/dpnp_upgrade
Aug 7, 2025
Merged

[CI] upgrade dpctl/dpnp versions#2414
icfaust merged 71 commits into
uxlfoundation:mainfrom
icfaust:dev/dpnp_upgrade

Conversation

@icfaust
Copy link
Copy Markdown
Contributor

@icfaust icfaust commented Apr 8, 2025

Description

This upgrades the version of dpnp and dpctl based off the recent release. This will notably add array_api support to dpnp.

This PR does the following:

  • Readd Python 3.12 to conda environment linux dpctl/dpnp testing
  • Makes clear the installation versions of dpctl, dpnp and the dpcpp runtime.
  • Adds renovatebot hooks to prevent this circumstance from occurring again
  • Update the DPC++ compiler to 2025.2
  • Adds a special wrapper (support_sycl_format) to move sycl data to cpu before validate_data is called, as the __array__ attribute has become blocked
  • Upgrade dpctl to 0.20.1 and dpnp to 0.18.1
  • Remove the special pytorch XPU install as the compiler upgrade breaks compatibility with it.
  • simplify the conversion of oneDAL tables to sycl_usm datatypes, changes in dpnp prevent easy access of zero copy support.
  • Fix lazy import to_dpnp function to use now that the previous method has become blocked for data zero copy of dpnp data, use it instead.
  • Fix mistake in array_to_usm recursion
  • Prevent return conversion of sparse datatypes, which occurs more often in lower priority estimator methods like kneighbors_graph
  • Add a test to make sure that dpctl and dpnp do not get silently removed from testing (which was the case before this update) making data framework testing intentional, even though data framework support is passive in the codebase
  • Wrap all inherited methods with support_input_format or support_sycl_format in order to guarantee sycl support with and without array api support (because of the blocking of np.array)
  • Move an incorrectly implemented validate_data call from EmpiricalCovariance.fit which should occur after dispatching.

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).

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2025

Codecov Report

❌ Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
onedal/_device_offload.py 83.33% 2 Missing and 2 partials ⚠️
Flag Coverage Δ
azure 80.95% <93.44%> (+0.15%) ⬆️
github 73.22% <91.80%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
onedal/datatypes/__init__.py 100.00% <100.00%> (ø)
onedal/datatypes/_data_conversion.py 80.00% <100.00%> (-1.09%) ⬇️
onedal/datatypes/_sycl_usm.py 80.64% <100.00%> (-1.41%) ⬇️
onedal/utils/_array_api.py 90.90% <100.00%> (+0.66%) ⬆️
sklearnex/_device_offload.py 87.17% <100.00%> (ø)
sklearnex/cluster/k_means.py 89.72% <100.00%> (+0.07%) ⬆️
sklearnex/covariance/incremental_covariance.py 90.14% <100.00%> (-1.29%) ⬇️
sklearnex/decomposition/pca.py 88.60% <100.00%> (+0.17%) ⬆️
sklearnex/ensemble/_forest.py 79.70% <100.00%> (+0.18%) ⬆️
sklearnex/linear_model/coordinate_descent.py 100.00% <100.00%> (ø)
... and 3 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@icfaust
Copy link
Copy Markdown
Contributor Author

icfaust commented Apr 8, 2025

This PR must wait until 2025.1 is available on apt for Linux. Then corrections to the codebase must be completed

@icfaust icfaust closed this Aug 4, 2025
@icfaust icfaust reopened this Aug 4, 2025
@icfaust
Copy link
Copy Markdown
Contributor Author

icfaust commented Aug 7, 2025

/intelci: run

Comment thread onedal/_device_offload.py Outdated
Comment thread onedal/datatypes/_data_conversion.py
Comment thread .github/workflows/ci.yml
@icfaust
Copy link
Copy Markdown
Contributor Author

icfaust commented Aug 7, 2025

/intelci: run

@icfaust
Copy link
Copy Markdown
Contributor Author

icfaust commented Aug 7, 2025

/intelci: run

return X

_transform = support_input_format(_sklearn_KMeans._transform)
transform = support_input_format(_sklearn_KMeans.transform)
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.

Was this meant to replace the line before?

Copy link
Copy Markdown
Contributor Author

@icfaust icfaust Aug 7, 2025

Choose a reason for hiding this comment

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

Unfortunately no ☹️ . This is an ugly solution based on how things have been implemented before this, KMeans.transform has its validate_data in a separate function, and then calls a function KMeans._transform . It was done this way on their side to do fit_transform, allowing for the second valdiation to be removed. We reuse _transform in our fit_transform implementation too, so I can't remove it there, but I need make transform work. I couldn't swap in a dispatch because we don't accelerate anything there with oneDAL in either case. Direct use of KMeans.transform using dpctl 0.18 is likely to be extremely slow. In general this is because its not a high-prority method, which we technically support but in a very bad way/ without benchmarking.

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.

Perhaps that could be left as a comment for when it comes the time to refactor again.

Copy link
Copy Markdown
Contributor

@ahuber21 ahuber21 left a comment

Choose a reason for hiding this comment

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

Approving, but subject to green CI (GH actions & pre-commit). I'm okay with ugly solutions for broken tests as long as we follow up. Let's organize that offline.

@icfaust
Copy link
Copy Markdown
Contributor Author

icfaust commented Aug 7, 2025

/intelci: run

Copy link
Copy Markdown
Contributor

@david-cortes-intel david-cortes-intel left a comment

Choose a reason for hiding this comment

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

LGTM, pending CI results.

@icfaust icfaust merged commit 6470324 into uxlfoundation:main Aug 7, 2025
26 of 32 checks passed
@icfaust icfaust deleted the dev/dpnp_upgrade branch August 7, 2025 13:19
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.

3 participants