-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[train] Simplify ray.train.xgboost/lightgbm
(3/n): Re-implement LightGBMTrainer
as a lightweight DataParallelTrainer
#43244
Conversation
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neato!
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Justin Yu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM : )
from ray.train.lightgbm.lightgbm_checkpoint import LightGBMCheckpoint | ||
from ray.train.lightgbm.lightgbm_predictor import LightGBMPredictor | ||
from ray.train.lightgbm.lightgbm_trainer import LightGBMTrainer | ||
|
||
__all__ = [ | ||
"RayTrainReportCallback", | ||
"LightGBMCheckpoint", | ||
"LightGBMConfig", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Are we exposing this LightGBMConfig
as a public API? Seems no special logics in this config, but it's still an argument in the LightGBMTrainer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah there's not really a reason for a user to know about LightGBMConfig
.
I was trying to keep the API consistent for LightGBMTrainer
with all the other DataParallelTrainer
s that take in a backend config. So, LightGBMConfig
would need to be public for it to be a constructor argument of v2.LightGBMTrainer
.
Let me remove it from the public ray.train.lightgbm
namespace for now while v2.LightGBMTrainer
is still private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these changes needed?
This PR re-implements
LightGBMTrainer
as aDataParallelTrainer
that does not uselightgbm_ray
under the hood, in an effort to unify the trainer implementations and remove that external dependency.This PR is the
lightgbm
counterpart to #42767. See that PR's description for details.Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.