Merging XGBoost changes from 2.4 #2712
Conversation
nvidianz
requested review from
chesterxgchen,
YuanTingHsieh,
SYangster,
yanchengnv and
ZiyueXu77
nvflare/app_opt/xgboost/histogram_based_v2/adaptors/grpc_client_adaptor.py
Outdated
Show resolved
Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/adaptors/grpc_client_adaptor.py
Outdated
Show resolved
Hide resolved
nvflare/app_opt/xgboost/histogram_based_v2/runners/xgb_client_runner.py
Outdated
Show resolved
Hide resolved
| @@ -72,11 +71,15 @@ def initialize(self, fl_ctx: FLContext): | |||
| if not isinstance(self._metrics_writer, LogWriter): | |||
| self.system_panic("writer should be type LogWriter", fl_ctx) | |||
|
|
|||
| def _xgb_train(self, params: XGBoostParams, train_data: xgb.DMatrix, val_data) -> xgb.core.Booster: | |||
| def _xgb_train(self, num_rounds, xgb_params: dict, xgb_options: dict, train_data, val_data) -> xgb.core.Booster: | |||
There was a problem hiding this comment.
for all the parameters to be passed into xgb.train method.
suggest either totally pack it as before a custom class call "XGBoostParams"
Or totally unpack it, so this method signature is similar to https://xgboost.readthedocs.io/en/stable/python/python_api.html#xgboost.train and docstring to explain each of the argument meaning.
Randomly group some into xgb_params, some standalone and some other into xgb_options is very confusing.
There was a problem hiding this comment.
The grouping is very clear. The params (Booster parameters) is passed as its own dictionary, everything else is packed into xgb_options. The params is specifically unpacked due to its importance.
There was a problem hiding this comment.
Got it, then suggest either rename to "num_boost_round" or remove this "num_round" argument to be consistent
Description
Merged all the changes made to XGBoost in 2.4 back to main. This includes the user experience improvement and several bug fixes.
Types of changes
./runtest.sh.