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

[CI, enhancement] enable external oneDAL builds in build-and-test-lnx.yml Azure Pipelines #2125

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Oct 22, 2024

Description

Try to add ability to externally specify a oneDAL build into this template. This is the beginning of a multi-step process to allow for the use of nightly oneDAL builds.


Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • 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 update 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 added a respective label(s) to PR if I have a permission for that.
  • 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 summary table with measured data, if performance change is expected.
  • I have provided justification why performance has changed or why changes are not expected.
  • I have provided justification why quality metrics have changed or why changes are not expected.
  • I have extended benchmarking suite and provided corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@icfaust icfaust changed the title [WIP] enable external dal builds in build-and-test-lnx.yml Azure Pipelines [WIP] enable non dal-devel builds in build-and-test-lnx.yml Azure Pipelines Oct 22, 2024
@icfaust icfaust changed the title [WIP] enable non dal-devel builds in build-and-test-lnx.yml Azure Pipelines [WIP] enable external oneDAL builds in build-and-test-lnx.yml Azure Pipelines Oct 22, 2024
@icfaust icfaust changed the title [WIP] enable external oneDAL builds in build-and-test-lnx.yml Azure Pipelines [CI, enhancement] enable external oneDAL builds in build-and-test-lnx.yml Azure Pipelines Oct 23, 2024
@icfaust icfaust marked this pull request as ready for review October 24, 2024 12:27
@icfaust icfaust marked this pull request as draft November 3, 2024 00:09
@icfaust icfaust marked this pull request as ready for review November 3, 2024 09:22
conda-recipe/build.sh Show resolved Hide resolved
@@ -27,47 +28,73 @@ steps:
conda config --add channels conda-forge
conda config --set channel_priority strict
conda update -y -q conda
conda create -q -y -n CB -c conda-forge python=$(PYTHON_VERSION) dal-devel mpich pyyaml "dpcpp-cpp-rt=2024.2.0"
conda create -q -y -n CB -c conda-forge python=$(PYTHON_VERSION) mpich pyyaml "dpcpp-cpp-rt=2024.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: would this switch all of these jobs to the external builds? I think it'd be quite useful to leave some with the stable release too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very good question and suggestion. Why do we stop using stable version for building and testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will switch to external builds only if a DALROOT is specified, and example using it is here: oneapi-src/oneDAL#2950 which is the immediate use case. The question remains the same as when GitHub actions CI was introduced, do we eventually want to move toward using versions of oneDAL main branch rather than release branches?

Copy link
Contributor

@samir-nasibli samir-nasibli Nov 11, 2024

Choose a reason for hiding this comment

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

@icfaust I think it would be good to get @homksei @Alexsandruss @napetrov and @syakov-intel ’s opinions on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samir-nasibli Just as a note, there is a precedence within the conda-recipe for DPCPPROOT set outside of the conda-recipe: https://github.com/intel/scikit-learn-intelex/blob/main/conda-recipe/build.sh#L38

This work simply continues on that precedent.

Secondly, the core flow using stable versions for building and testing on azure pipelines CI is not changed. Dal-devel and oneapi components are still acquired from conda in azure pipelines.

Copy link
Contributor

@samir-nasibli samir-nasibli Nov 20, 2024

Choose a reason for hiding this comment

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

Thank you @icfaust for the clarification. For me resolved.

Copy link
Contributor

@samir-nasibli samir-nasibli left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!
Depending on @david-cortes-intel @ethanglaser @Alexsandruss reviews

Copy link
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.

Just a comment that perhaps once this PR gets merged: #2170
There might be an easier way to avoid these .sh scripts with external builds.

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