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

[c++] fix parallel_tree_learner_split_info #6738

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

moming39
Copy link
Contributor

@moming39 moming39 commented Dec 7, 2024

#6491 (comment)
This code is used for computing the buffer size of the communicating the split info the in distributed training. But lost the two most recently added parameters(right_sum_gradient_and_hessian, left_sum_gradient_and_hessian) size. When use parallel_tree_learner method with number of categories of one feature larger than 28 (default max_cat_to_onehot(32)-2*2), it will resut in an Segmentation fault.

microsoft#6491 (comment)
This code is used for computing the buffer size of the communicating the split info the in distributed training. But lost the two most recently added parameters(right_sum_gradient_and_hessian, left_sum_gradient_and_hessian) size.
When use parallel_tree_learner method with number of categories of one feature larger than 28 (default max_cat_to_onehot(32)-2*2), it will resut in an Segmentation fault.
@jameslamb jameslamb changed the title fix_ parallel_tree_learner_split_info [c++] fix parallel_tree_learner_split_info Dec 7, 2024
@jameslamb
Copy link
Collaborator

@shiyu1994 can you please review this? I'm not confident I understand the changes, and not sure if a similar fix is needed in other places.

Copy link
Collaborator

@shiyu1994 shiyu1994 left a comment

Choose a reason for hiding this comment

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

Thank you for finding out this issue.

@MelleVessies
Copy link

Thanks for the quick merge! Everything seems to be working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants