Update of DPC dependencies for CI and conda-recipe #2160
Conversation
Alexsandruss
changed the title
Alexsandruss
added
infra
dependencies
|
/intelci: run |
Alexsandruss
requested review from
napetrov,
homksei,
ahuber21 and
ethanglaser
as code owners
.ci/pipeline/build-and-test-lnx.yml
Outdated
| @@ -48,7 +45,7 @@ steps: | |||
| bash .ci/scripts/setup_sklearn.sh $(SKLEARN_VERSION) | |||
| pip install --upgrade -r requirements-test.txt | |||
| pip install $(python .ci/scripts/get_compatible_scipy_version.py) | |||
| if [ $(echo $(PYTHON_VERSION) | grep '3.9\|3.11') ] && [ $(SKLEARN_VERSION) != "1.0" ]; then conda install -q -y -c https://software.repos.intel.com/python/conda/ dpctl=0.17.0 dpnp=0.15.0; fi | |||
| if [ $(SKLEARN_VERSION) != "1.0" ]; then pip install dpctl==0.18.* dpnp==0.16.*; fi | |||
There was a problem hiding this comment.
Are we installing dpctl/dpnp for all py versions?
There was a problem hiding this comment.
Yes, it has same supported python versions as sklearnex.
There was a problem hiding this comment.
that does, however, cause certain functionality within sklearnex to become untested (see DummySyclQueue)
There was a problem hiding this comment.
Is DummySyclQueue needed if dpctl supports all sklearnex versions?
There was a problem hiding this comment.
DummySyclQueue is needed for device offloading, if dpctl is not available in env.
There was a problem hiding this comment.
Are there circumstances at this point where dpctl would not be available?
There was a problem hiding this comment.
I can give at least two examples:
- Installation Issues: problems on the user’s side during the installation of dpctl.
- Environment Compatibility: for example, dpctl does not currently support Python 3.13. While scikit-learn already supports Python 3.13 and we may soon add it to our matrices, dpctl will not be available for this Python version at this time.
This reverts commit 6615964.
|
/intelci: run |
|
/intelci: run |
conda-recipe/meta.yaml
Outdated
| - numpy {{ numpy }} | ||
| - impi-devel # [not win] | ||
| - numpy | ||
| - mpich # [not win] |
There was a problem hiding this comment.
Why mpich instead of impi-devel?
There was a problem hiding this comment.
It was for testing purposes, mpi version doesn't make difference in public build of conda-recipe.
| - {{ pin_compatible('numpy') }} | ||
| - dpcpp-cpp-rt # [linux64] | ||
| - numpy | ||
| - scikit-learn |
There was a problem hiding this comment.
Also, is scikit-learn needed in build deps or should it remain as test dep?
There was a problem hiding this comment.
scikit-learn is only run time dependency.
There was a problem hiding this comment.
Before merge, @Alexsandruss could you leave a comment in the PR on the switch from conda to pip for dpctl/dpnp? This way when we face an upgrade issue with dpctl/dpnp in the future there is a bit of a note for someone to understand (doesn't need to be in the codebase, but something like research via git-blame to this PR would be nice)
|
Also thank you for doing this, very much appreciated @Alexsandruss |
Description
Changes:
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
N/A, CI update.