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

oneMKL migration: Fix for DLL loading issue on Windows #2063

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Vika-F
Copy link
Contributor

@Vika-F Vika-F commented Sep 20, 2024

__init__.py file in daal4py module was modified to add paths to TBB DLLs into the process environment in case TBBROOT environment variable is available.

This work is related to oneDAL PR #2756.


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

  • The unit tests pass successfully.
  • I have run it locally and tested the changes extensively.

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.

@Vika-F Vika-F added the infra label Sep 24, 2024
@Vika-F Vika-F marked this pull request as ready for review September 24, 2024 08:38
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.

@Vika-F please provide the link to the job with your branch and oneapi-src/oneDAL#2756.

@Vika-F
Copy link
Contributor Author

Vika-F commented Sep 24, 2024

@Vika-F Vika-F changed the title [WIP] oneMKL migration: Fix for DLL loading issue on Windows oneMKL migration: Fix for DLL loading issue on Windows Sep 24, 2024
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.

@Vika-F Thank you for the work done!
No objections from my side.
Before to approve let me ask: is this one of the options or agreed fix for OneMKL issues on sklearnex side?

@samir-nasibli
Copy link
Contributor

@samir-nasibli CI job: http://intel-ci.intel.com/ef7749d8-99a3-f16f-a475-a4bf010d0e2d

Expect that test fails covered by #2066 deselections

@Vika-F
Copy link
Contributor Author

Vika-F commented Sep 25, 2024

@samir-nasibli yes, those changes were agreed with @napetrov and @syakov-intel .

@samir-nasibli samir-nasibli merged commit 65edc89 into intel:main Sep 25, 2024
23 checks passed
Vika-F added a commit to Vika-F/scikit-learn-intelex that referenced this pull request Sep 26, 2024
…ipt (intel#2063)

* oneMKL migration: Fix for DLL loading issue on Windows
napetrov pushed a commit that referenced this pull request Oct 3, 2024
…25.0 release branch (#2070)

* oneMKL migration: Temporary deselection of tests  (#2066)

* Temporary deselection of tests that started to fail after oneMKL migration

* Moved deseletions for 'onedal' and 'sklearnex' modules directly into test codes

* minor change

* Replace TODO notes with pytest.skip

* Deselect test_naive_bayes and test_naive_bayes_streaming in daal4py TestExCSRMatrix

* DEV: Add path to tbb*.dll into environment in daal4py module init script (#2063)

* oneMKL migration: Fix for DLL loading issue on Windows
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.

2 participants