Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

fix nthread param typo #16

Merged
merged 1 commit into from
Nov 30, 2017
Merged

fix nthread param typo #16

merged 1 commit into from
Nov 30, 2017

Conversation

evanfwelch
Copy link
Contributor

I was having issues with distributed training where I was seeing a race condition develop. As soon as I manually implemented nthread= i realized that core.py improperly sets this parameter as nthreads and therefore xgboost ignores it and goes to the default behavior.

I was having issues with distributed training where I was seeing a race condition develop. As soon as I manually implemented nthread=<available threads> i realized that `core.py` improperly sets this parameter as `nthreads` and therefore xgboost ignores it and goes to the default behavior.
@mrocklin
Copy link
Member

Confirmed. Thank you for finding this. I'm curious, do you know of any way to check the parameters we send to XGBoost before passing them? Is there anything we can do to avoid situations like this in the future?

@evanfwelch
Copy link
Contributor Author

Thanks Matt. (By the way, the package is great and I love quickly you got it out the door). I dont know of anything on the XGBoost side that helps us check the parameters we're sending. But perhaps, given the fact that dask_xgboost supports a subset of full XGBoost functionality (e.g. you can't yet pass eval data to facilitate early stopping) we could build in a sort of definitive list of options and then check the keys of the params and kwargs against it. If we throw warning when users give unsupported params it might make the package a lot more usable. I had to do a bit of digging to figure out that adding evaluation data was not going to work (since you can't pickle a DMatrix).

@mrocklin
Copy link
Member

OK, if there is no automated way to verify keywords then lets merge this for now. Thanks for finding and fixing this @evanfwelch

@mrocklin mrocklin merged commit b6b6b97 into dask:master Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants