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

[python-package] fix sklearn n_jobs/nthreads and seed/random_state bug #2378

Merged
merged 3 commits into from Jun 12, 2017

Conversation

Projects
None yet
4 participants
@wxchan
Contributor

wxchan commented Jun 4, 2017

discuss in 6cea1e3#commitcomment-22391065

if n_jobs/nthreads or seed/random_state is changed in other place except __init__, it will cause clone error.

@wxchan wxchan changed the title from [python-package] fix sklearn n_jobs/nthreads and seed/random_state issue to [python-package] fix sklearn n_jobs/nthreads and seed/random_state bug Jun 4, 2017

@wxchan

This comment has been minimized.

Show comment
Hide comment
@wxchan

wxchan Jun 4, 2017

Contributor

@ClimbsRocks check this pr, I think it will fix the error you mention.

Contributor

wxchan commented Jun 4, 2017

@ClimbsRocks check this pr, I think it will fix the error you mention.

wxchan referenced this pull request Jun 4, 2017

Sklearn convention update (#2323)
* Added n_jobs and random_state to keep up to date with sklearn API.
Deprecated nthread and seed.  Added tests for new params and
deprecations.

* Fixed docstring to reflect updates to n_jobs and random_state.

* Fixed whitespace issues and removed nose import.

* Added deprecation note for nthread and seed in docstring.

* Attempted fix of deprecation tests.

* Second attempted fix to tests.

* Set n_jobs to 1.
@gaw89

gaw89 suggested changes Jun 5, 2017 edited

This looks great @wxchan! Thanks for getting on it so quickly. Could I request that we also fix the warnings in this PR? If we change the line:

warnings.simplefilter('default', DeprecationWarning)

to:

warnings.filterwarnings('default', '.+nthread.+|.+seed.+', DeprecationWarning)

It will ensure that only those two warnings are raised. Currently, that line may inadvertently cause warnings in other packages to be raised.

@wxchan

This comment has been minimized.

Show comment
Hide comment
@wxchan

wxchan Jun 5, 2017

Contributor

@gaw89 I didn't find warnings.simplefilter('default', DeprecationWarning). Do you mean changing Line 16 https://github.com/wxchan/xgboost/blob/36797008735be7b199adee8e3ff95bc160f558b8/python-package/xgboost/sklearn.py#L16? warnings.simplefilter('always', DeprecationWarning) to warnings.filterwarnings('default', '.+nthread.+|.+seed.+', DeprecationWarning)?

Contributor

wxchan commented Jun 5, 2017

@gaw89 I didn't find warnings.simplefilter('default', DeprecationWarning). Do you mean changing Line 16 https://github.com/wxchan/xgboost/blob/36797008735be7b199adee8e3ff95bc160f558b8/python-package/xgboost/sklearn.py#L16? warnings.simplefilter('always', DeprecationWarning) to warnings.filterwarnings('default', '.+nthread.+|.+seed.+', DeprecationWarning)?

@gaw89

This comment has been minimized.

Show comment
Hide comment
@gaw89

gaw89 Jun 5, 2017

Contributor

@wxchan, sorry for being unclear. That is what I was meaning. Line 16. Perhaps this is bad practice to slip in a fix for multiple bugs under one PR, but it seemed like this might be a convenient place to fix it (rather than creating a whole PR for just one line). If it's a bad idea to make that fix here, I'm happy to submit it in a separate PR.

Contributor

gaw89 commented Jun 5, 2017

@wxchan, sorry for being unclear. That is what I was meaning. Line 16. Perhaps this is bad practice to slip in a fix for multiple bugs under one PR, but it seemed like this might be a convenient place to fix it (rather than creating a whole PR for just one line). If it's a bad idea to make that fix here, I'm happy to submit it in a separate PR.

@ClimbsRocks

This comment has been minimized.

Show comment
Hide comment
@ClimbsRocks

ClimbsRocks Jun 5, 2017

Contributor

Thanks for the prompt attention!

Thanks to @mglowacki100 's comment ClimbsRocks/auto_ml#232 (comment), i've got a folder that reproduces the error.
aml_test.zip

I didn't include XGBoost in there, and you'll have to pip install auto_ml, but otherwise it should be set.

Unfortunately, I tried checking out this commit and then rebuilding XGBoost, and it didn't seem to stop the error.

I found something interesting as well. I made a change to auto_ml master, where I removed all references to nthread, and still got the nthread deprecation warning.

I haven't tried building anything other than the master branch of xgb before, so i might have messed that part up, but hopefully this folder helps you reproduce the error!

Contributor

ClimbsRocks commented Jun 5, 2017

Thanks for the prompt attention!

Thanks to @mglowacki100 's comment ClimbsRocks/auto_ml#232 (comment), i've got a folder that reproduces the error.
aml_test.zip

I didn't include XGBoost in there, and you'll have to pip install auto_ml, but otherwise it should be set.

Unfortunately, I tried checking out this commit and then rebuilding XGBoost, and it didn't seem to stop the error.

I found something interesting as well. I made a change to auto_ml master, where I removed all references to nthread, and still got the nthread deprecation warning.

I haven't tried building anything other than the master branch of xgb before, so i might have messed that part up, but hopefully this folder helps you reproduce the error!

@wxchan

This comment has been minimized.

Show comment
Hide comment
@wxchan

wxchan Jun 12, 2017

Contributor

sorry I am outside these days.

@ClimbsRocks I cannot reproduce error with your provided codes in aml_test.zip. seem already solved.

@gaw89 I suggest opening another pr, because I am not quite familiar with python warnings, afraid to get something wrong.

btw, who should I call for a review for this thread?

Contributor

wxchan commented Jun 12, 2017

sorry I am outside these days.

@ClimbsRocks I cannot reproduce error with your provided codes in aml_test.zip. seem already solved.

@gaw89 I suggest opening another pr, because I am not quite familiar with python warnings, afraid to get something wrong.

btw, who should I call for a review for this thread?

@gaw89

This comment has been minimized.

Show comment
Hide comment
@gaw89

gaw89 Jun 12, 2017

Contributor

@wxchan, thanks, I will submit a separate PR. I would suggest asking @terrytangyuan for a review and to accept the PR.

Contributor

gaw89 commented Jun 12, 2017

@wxchan, thanks, I will submit a separate PR. I would suggest asking @terrytangyuan for a review and to accept the PR.

@terrytangyuan terrytangyuan merged commit 65d2513 into dmlc:master Jun 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

CodingCat added a commit to CodingCat/xgboost that referenced this pull request Jun 18, 2017

[python-package] fix sklearn n_jobs/nthreads and seed/random_state bug (
#2378)

* add a testcase causing RuntimeError

* move seed/random_state/nthread/n_jobs check to get_xgb_params()

* fix failed test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment