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

Implemented proper work with multiple threads #1361

Merged
merged 17 commits into from
Sep 12, 2023

Conversation

olegkkruglov
Copy link
Contributor

@olegkkruglov olegkkruglov commented Jul 13, 2023

Changes proposed in this pull request:

  • Add patching for _FuncWrapper class from sklearn.utils.parallel to properly call patched config_context
  • Fix patching for get_config to allow run patched version in sklearn.utils.parallel
  • Remove unnecessary queue propagation via config in BaseSVC._fit_proba which causes test failure after get_config patch fix

@olegkkruglov olegkkruglov force-pushed the parallel-fix branch 2 times, most recently from ed4ccb1 to 8907d32 Compare July 31, 2023 22:54
@samir-nasibli samir-nasibli reopened this Jul 31, 2023
@samir-nasibli
Copy link
Contributor

samir-nasibli commented Jul 31, 2023

In the next PRs please don't mix implementing common feature and bug fix. But for now we can leave it as is.
Better disable the test and fix it on separate branch :)

@olegkkruglov
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor

/intelci: run

@olegkkruglov
Copy link
Contributor Author

/intelci: run

1 similar comment
@olegkkruglov
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor

@olegkkruglov please rebase your branches and run intelci

@olegkkruglov
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor

/intelci: run

@ethanglaser
Copy link
Contributor

ethanglaser commented Sep 5, 2023

Any additional tests/examples for this functionality? Or is it covered by existing?

@samir-nasibli
Copy link
Contributor

samir-nasibli commented Sep 5, 2023

You can use this reproducer for the test:

import numpy as np
from sklearnex import patch_sklearn, config_context
from sklearn.datasets import make_classification
from sklearn.ensemble import BaggingClassifier
patch_sklearn()

from sklearn.svm import SVC

X, y = make_classification(
    n_samples=1000,
    n_features=4,
    n_informative=2,
    n_redundant=0,
    random_state=0,
    shuffle=False,
)

with config_context(target_offload="gpu"):
    ExtraTreesClassifier(max_depth=2, random_state=0).fit(X, y)
        # decision_function
    ensemble = BaggingClassifier(
        SVC(decision_function_shape="ovr"), n_jobs=3, random_state=0
    ).fit(X, y)

The result is:

Intel(R) Extension for Scikit-learn* enabled (https://github.com/intel/scikit-learn-intelex)
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU
INFO:sklearnex: sklearn.svm.SVC.fit: running accelerated version on GPU

On master it fallbacks on CPU. So your branch works correct with n_jobs enabled in this case.

@ethanglaser
Copy link
Contributor

On master it fallbacks on CPU. So your branch works correct with n_jobs enabled in this case.

Perhaps adding something similar with allow_fallback_to_host=False flag added to config_context would be a good test of whether things are working properly?

@samir-nasibli
Copy link
Contributor

On master it fallbacks on CPU. So your branch works correct with n_jobs enabled in this case.

Perhaps adding something similar with allow_fallback_to_host=False flag added to config_context would be a good test of whether things are working properly?

Good point, we can update logger for that as well.

@Alexsandruss
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Sep 7, 2023

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/7)
Auto-merging sklearnex/dispatcher.py
CONFLICT (content): Merge conflict in sklearnex/dispatcher.py
error: could not apply 284f7a7... Implemented proper work with multiple threads
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 284f7a7... Implemented proper work with multiple threads

@Alexsandruss
Copy link
Contributor

/intelci: run

def test_config_context_in_parallel():
x, y = make_classification(random_state=42)
try:
with config_context(target_offload="gpu"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with config_context(target_offload="gpu"):
with config_context(target_offload="gpu", allow_fallback_to_host=False):

Copy link
Contributor

@ethanglaser ethanglaser Sep 7, 2023

Choose a reason for hiding this comment

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

Actually maybe this modification isn't necessary, but I am a bit confused by the test - when would dpctl be available but no GPU? Thanks for adding the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

dpctl is not only for gpu devices. For example, CI has dpctl installed without gpu in azure pipelines used instances.

@Alexsandruss
Copy link
Contributor

/intelci: run

2 similar comments
@Alexsandruss
Copy link
Contributor

/intelci: run

@Alexsandruss
Copy link
Contributor

/intelci: run

@Alexsandruss
Copy link
Contributor

@samir-nasibli
Copy link
Contributor

Please attach GPU CI job as well.

@ethanglaser
Copy link
Contributor

Please attach GPU CI job as well.

http://intel-ci.intel.com/ee51704d-b860-f1a2-a5d9-a4bf010d0e2e

@napetrov
Copy link
Contributor

@Alexsandruss
Copy link
Contributor

@Alexsandruss should example will be updated https://github.com/intel/scikit-learn-intelex/blob/master/examples/sklearnex/n_jobs.py

Will be in next PR with n_jobs parameter update.

@Alexsandruss Alexsandruss merged commit 90e531e into intel:master Sep 12, 2023
16 of 17 checks passed
@@ -54,7 +54,9 @@ pytest --verbose --pyargs ${daal4py_dir}/daal4py/sklearn
return_code=$(($return_code + $?))

echo "Pytest of sklearnex running ..."
pytest --verbose --pyargs ${daal4py_dir}/sklearnex
# TODO: investigate why test_monkeypatch.py might cause failures of other tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have proper tracker for this issues?

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.

5 participants