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] use -fvisibility=hidden in Linux Make builds #3080

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

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Feb 18, 2025

Description

This PR uses __attribute__ ((visibility ("default"))) https://gcc.gnu.org/wiki/Visibility to make the oneDAL dynamic libraries in Linux follow Windows in having default visibility of symbols as hidden. This will make the Linux ABI explicit, and make possible improvements in use of the oneDAL library as described in the linked article. In general, most problems are related to implicit Instantiation, which doesn't respect the added attribute for visibility.

NOTE: Building with Bazel still maintains standard linux visibility, the change in visibility only occurs in Make builds.

The changes focus in several overarching themes:

  1. DAAL Implicit Instantiation of the AlgorithmDispatchContainer is removed by moving all Batch, Online and Distributed object constructors from the class definition to individual definitions in the *_dispatch.cpp files. This follows convention set by some algorithms: KNN, stump, etc. Follow up work is recommended to solve public exposition and requirement of an internal namespace in SVM which violates convention.

  2. DAAL Linear model interpolation cannot follow the convention in 1) due to certain re-use of the AlgorithmDispatchContainer, instead the initialize method is moved to *_dispatch.cpp for Linear Regression, Logistic Regression, Lasso, Ridge, and ElasticNet. A note is made on this change in the codebase, as it is a special case of a reuse of another algorithms AlgorithmDispatchContainer.

  3. DAAL RNG engine instantion of the object is moved from a general .cpp file to just the create function, and the other instantiation of the contructors are done following the methodology laid out in 1)

  4. DAAL An outlier file in Expectation Maximization is removed (which is the implementation of 1) in the wrong file and follows now convention set out in 1) using *_dispatcher.cpp.

  5. DAAL Macro definition is changed for AlgorithmDispatchContainer to guarantee that it is exported for various architectures for Batch computations.

  6. General changes to the makefile and DAAL_EXPORT/ ONEDAL_EXPORT are made to generalize and activate the default hidden visibility. Subtle logic changes are made

  7. oneDAL ONEDAL_EXPORT is added in several places where the logic differs between windows use of __declspec(dllexport) and __attribute__ ((visibility ("default"))), which varies by compiler and OS to make it valid for all circumstances.

  8. oneDAL some implicit instantiations are fixed. (See incremental algorithms)

  9. oneDAL ONEDAL_EXPORT for spmd is properly addressed which should make it possible to use with windows OS (not that this is recommended)

Note: this PR has been additionally tested against a main sklearnex build to verify conformance.

This reduces the dynamic table length in Linux by 10x in DAAL, and x5 overall.


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

  • 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 Feb 18, 2025
@icfaust
Copy link
Contributor Author

icfaust commented Mar 5, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 5, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust icfaust changed the title [WIP, CI, enhancement] enforce ABI checking of linux DPCPP build [WIP, CI, enhancement] use -fvisibility=hidden in Linux builds Mar 10, 2025
@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust icfaust changed the title [WIP, CI, enhancement] use -fvisibility=hidden in Linux builds [WIP, CI, enhancement] use -fvisibility=hidden in Linux Make builds Mar 10, 2025
@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

more advanced CI run showing no problems (ignore infrastructure issues, tests still ran through properly) http://intel-ci.intel.com/effdbfb8-1661-f1b1-9595-a4bf010d0e2d

@icfaust
Copy link
Contributor Author

icfaust commented Mar 10, 2025

/intelci: run

@icfaust icfaust changed the title [WIP, CI, enhancement] use -fvisibility=hidden in Linux Make builds [CI, enhancement] use -fvisibility=hidden in Linux Make builds Mar 10, 2025
@icfaust icfaust marked this pull request as ready for review March 10, 2025 18:22
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.

1 participant