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] changes for GitHub actions PR CI for matching oneDAL Nightly-build #2076

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

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Sep 30, 2024

Description

Fixes to return CI to green as Nightly-build was changed in oneapi-src/oneDAL#2865 and oneapi-src/oneDAL#2920

No operational changes made, thus benchmarking unnecessary.


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
Copy link
Contributor Author

icfaust commented Oct 2, 2024

Still a problem with kneighbors and stability, this probably will need to be deselected and a ticket made for investigation.

@icfaust
Copy link
Contributor Author

icfaust commented Oct 2, 2024

Still to be done: ticket for investigations on kneighbors stability issues. Ideally the CI should be re-run 8-10x to make sure I caught all the intermittent failures. I need to add the additional sparsity deselections to the sparsity investigation ticket, and is exhibited both in Intel and non-Intel hardware.

@icfaust icfaust added bug Something isn't working infra labels Oct 2, 2024
@icfaust icfaust marked this pull request as ready for review October 2, 2024 13:08
@icfaust
Copy link
Contributor Author

icfaust commented Oct 2, 2024

/intelci: run

deselected_tests.yaml Outdated Show resolved Hide resolved
sklearnex/cluster/tests/test_kmeans.py Outdated Show resolved Hide resolved
sklearnex/tests/utils/base.py Outdated Show resolved Hide resolved
@icfaust
Copy link
Contributor Author

icfaust commented Oct 2, 2024

/intelci: run

@icfaust
Copy link
Contributor Author

icfaust commented Oct 2, 2024

/intelci: run

Comment on lines +262 to +269
if (
not _IS_INTEL
and ("Neighbors" in estimator or "LocalOutlierFactor" in estimator)
and method in ["score", "predict", "kneighbors", "kneighbors_graph"]
):
if daal_check_version((2025, "P", 200)):
pytest.fail("Re-verify failure of algorithms in 2025.2 oneDAL")
pytest.skip(f"{estimator} shows instability on Non-Intel hardware")
Copy link
Contributor

@samir-nasibli samir-nasibli Oct 2, 2024

Choose a reason for hiding this comment

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

I propose to make a common function for checking and disabling as the nature of these crashes is the same.
Also less code duplication

def _skip_neighbors(estimator, method):
    if (
        not _IS_INTEL
        and ("Neighbors" in estimator or "LocalOutlierFactor" in estimator)
        and method in ["score", "predict", "kneighbors", "kneighbors_graph"]
    ):
        if daal_check_version((2025, "P", 200)):
            pytest.fail("Re-verify failure of algorithms in 2025.2 oneDAL")
        pytest.skip(f"{estimator} shows instability on Non-Intel hardware")
Suggested change
if (
not _IS_INTEL
and ("Neighbors" in estimator or "LocalOutlierFactor" in estimator)
and method in ["score", "predict", "kneighbors", "kneighbors_graph"]
):
if daal_check_version((2025, "P", 200)):
pytest.fail("Re-verify failure of algorithms in 2025.2 oneDAL")
pytest.skip(f"{estimator} shows instability on Non-Intel hardware")
_skip_neighbors(estimator, estimator)

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.

@icfaust thank you for the work done! Seems we have green ci now.
#2081 is already merged. Please rebase and address comments. Good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants