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

[fix] remove vestigial Python code and define dependency for oneDAL make builds #2929

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Oct 1, 2024

Description

Building on older ubuntu versions fail due to deploy/generate_pkgconfig.py being Python 3.6+ dependent. This was not standard until Ubuntu 22. These changes make it buildable on Ubuntu versions which maintain a python version (not explicitly Python3). Requiring Python is not included in the INSTALL.md, is set but without a version.

NOTE: All scripts under .ci/scripts/conformance-scripts/ are vestigial, and can be removed. (Are done so in this PR).


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 added the infra label Oct 1, 2024
@icfaust icfaust changed the title [fix] address unwritten Python dependency in oneDAL building [fix] address unwritten Python dependency in oneDAL make builds Oct 1, 2024
@icfaust icfaust marked this pull request as ready for review October 1, 2024 15:12
@icfaust
Copy link
Contributor Author

icfaust commented Oct 1, 2024

/intelci: run

INSTALL.md Outdated
@@ -22,6 +22,7 @@ Required Software:
* [DPC++ Compiler](https://www.intel.com/content/www/us/en/developer/tools/oneapi/dpc-compiler.html)
* Microsoft Visual Studio\* (Windows\* only)
* [MSYS2](http://msys2.github.io) (Windows\* only)
* Python
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a line about it in the current master branch, two lines above where this would be inserted:
https://github.com/oneapi-src/oneDAL/blob/66f4858285c229a44be2ab17843edcbafaa989b9/INSTALL.md?plain=1#L23

It does mention "3.9" though so I guess there's no need to forego f-strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for clarity, the mentioning of 3.9 was just introduced in your PR #2923 just after this PR was submitted (hence why I was confused). 3.9 requirement is set from here #2923 (comment) . I will rebase this PR to reflect your changes.

The only python script that is run for make install is this python file and is the source of the dependency. We do not test python consistently in any of our build steps. The private CI uses 3.12 as defined by infrastructure. The Ubuntu 24.04 docker runners use 3.12. The azure pipelines runners are also using default Ubuntu 22.04 python (3.10) runners. We do not test 3.9 nor 3.11 for oneDAL building.

Thus by changing these f-strings, we can support Ubuntu 20+ default python (i.e. python 2.7+).

@napetrov is there a reason why on build-side for oneDAL there is this python requirement?

@icfaust icfaust changed the title [fix] address unwritten Python dependency in oneDAL make builds [fix] remove vestigial Python code and define dependency for oneDAL make builds Oct 7, 2024
@icfaust
Copy link
Contributor Author

icfaust commented Oct 7, 2024

I have removed vestigial code from the codebase which does not get run in any CI step any longer.

@icfaust icfaust marked this pull request as draft October 7, 2024 12:57
@icfaust
Copy link
Contributor Author

icfaust commented Oct 7, 2024

Move back to draft because sklearn conformance in this repo absolutely needs to be updated. On the border of disappointing.

@napetrov
Copy link
Contributor

/azp run CI

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants