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

[Breaking] Remove rabit support for custom reductions and grow_local_histmaker updater #7992

Merged
merged 7 commits into from Jun 21, 2022

Conversation

rongou
Copy link
Contributor

@rongou rongou commented Jun 14, 2022

Right now cpu tree methods rely on rabit::Reducer to perform custom reductions, but in reality they are just sums of gradients and hessians. Neither NCCL nor the new federated learning gRPC supports custom reductions, so removing this would move us closer to a common interface.

Additionally rabit::SerializeReducer supports reduction on arbitrary data structure, but it's only used by updater_histmaker.cc, which is no longer being referenced anywhere in the code base, so both are removed.

@rongou
Copy link
Contributor Author

rongou commented Jun 14, 2022

@trivialfis @RAMitchell

Not sure if this is the best approach, please take a look and let me know what you think. Next step I need to do some deeper refactoring to get rid of rabit::SerializeReducer.

@rongou rongou changed the title Remove rabit reducer Remove rabit support for custom reductions Jun 16, 2022
@rongou
Copy link
Contributor Author

rongou commented Jun 16, 2022

@trivialfis @RAMitchell turns out rabit::SerializeReducer is only used by updaters_histmaker.cc, which doesn't seem to be in use any more, so removed both.

I'm not sure why the R tests are failing. Is it caused by this change?

@trivialfis
Copy link
Member

The R test is unrelated. I'm trying to unblock the CI.

@trivialfis
Copy link
Member

I'm happy to remove the hist maker. It's old code to implement global_approx and local_approx. I rewrote the former but the latter was kept in this hist_maker. We don't usually use it but I mentioned it in the doc https://github.com/dmlc/xgboost/blob/master/doc/treemethod.rst#approximated-solutions so it's a breaking change non the less. Please annotate the PR with [breaking] and mention the change in the title so that we can include the change in release news.

@rongou rongou changed the title Remove rabit support for custom reductions [Breaking] Remove rabit support for custom reductions and grow_local_histmaker updater Jun 17, 2022
@rongou
Copy link
Contributor Author

rongou commented Jun 17, 2022

@trivialfis done.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase to master branch.

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.

None yet

2 participants