Prepare RF split counts for distributed training

[RAMitchell]

Description

This PR prepares the random forest split representation for distributed tree building by separating the left-child sample count into two explicit fields:

• global_nLeft: total number of samples assigned to the left child
• local_nLeft: rank-local number of samples assigned to the left child

For the current single-GPU/single-rank path, these values are intentionally the same. The objective call sites pass nLeft, nLeft explicitly so that staging assumption is visible in the code instead of hidden behind a convenience constructor.

This also removes the duplicate host-side HostSplit mirror used by tests. To make the real DT::Split type usable in host-side test code, its plain value-type constructors and copy assignment are now HDI; CUDA-specific methods remain device-only.

Motivation

Distributed random forest training needs to distinguish the global split count used for tree metadata from the local split count used for partitioning this rank’s rows. This PR introduces that distinction without changing current single-rank behavior.

Testing

• Built SG_RF_TEST
• Ran focused tests:
  • RFEquivalentSplitRangeTest.*
  • RfTest.EquivalentSplitRangePersistsThroughBuilder
• Ran full SG_RF_TEST: 289/289 passed

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[Copilot]

Pull request overview

This PR prepares the batched-level decision tree/random forest split representation for future distributed tree building by splitting the left-child sample count into separate global vs rank-local fields, while preserving current single-rank behavior.

Changes:

• Replaced nLeft with global_nLeft and local_nLeft in DT::Split, updating merge/reduction logic accordingly.
• Updated objective and builder/kernel call sites to compute/propagate 64-bit counts and to pass (nLeft, nLeft) explicitly for the single-rank path.
• Simplified tests and host-side handling by using DT::Split directly instead of a host mirror struct.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
cpp/tests/sg/rf_test.cu	Updates split assertions to validate both global_nLeft and local_nLeft and removes the test-only host mirror.
cpp/src/decisiontree/batched-levelalgo/split.cuh	Extends Split with global/local left counts and updates reduction/printing paths.
cpp/src/decisiontree/batched-levelalgo/objectives.cuh	Switches split-count plumbing to std::int64_t and updates split construction to supply both counts explicitly.
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh	Updates split partition validation to use local_nLeft with 64-bit-safe comparisons.
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh	Updates partition scan state/counting to use 64-bit counts and indexes using local_nLeft.
cpp/src/decisiontree/batched-levelalgo/builder.cuh	Uses global_nLeft for tree metadata counts and local_nLeft for instance-range partitioning; removes host mirror in split retry flow.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1e8ac2d9-5b51-4749-9123-27f04aa4ab86

📥 Commits

Reviewing files that changed from the base of the PR and between da3a352 and b84c7f5.

📒 Files selected for processing (3)

• cpp/src/decisiontree/batched-levelalgo/builder.cuh
• cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh
• cpp/tests/sg/rf_test.cu

🚧 Files skipped from review as they are similar to previous changes (3)

• cpp/tests/sg/rf_test.cu
• cpp/src/decisiontree/batched-levelalgo/builder.cuh
• cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved decision tree training split counting and partition handling for both classification and regression, reducing inconsistencies in how left/right samples are derived and strengthening split validity checks.
  • Updated split gain calculations and output placement to use wider count arithmetic, improving correctness on larger datasets.
• Tests
  • Refreshed CUDA split-equivalence checks to match the corrected left-sample accounting, ensuring training-equivalent split ranges remain consistent.

Walkthrough

This PR replaces DT::Split::nLeft with separate global_nLeft and local_nLeft std::int64_t fields, then propagates the representation through gain computation, partition kernels, builder child construction, copy-back, and split-range tests.

Changes

Decision tree split accounting int64 refactor

Layer / File(s)	Summary
Split struct contract and helper changes cpp/src/decisiontree/batched-levelalgo/split.cuh	Split and CountLeft use std::int64_t, with updated split equivalence, reduction, split selection, and debug output.
Gain computation using int64 counts cpp/src/decisiontree/batched-levelalgo/objectives.cuh	Classification and regression gain declarations, leaf-size checks, and split updates use std::int64_t count parameters.
Partition kernel state and validity checks cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh, cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh	Partition scan state, writer placement, scan input generation, and validity checks use int64 counts and split.local_nLeft.
Builder tree construction and final split copy-back cpp/src/decisiontree/batched-levelalgo/builder.cuh	Child global and local ranges are derived from the two split counts, and final splits are stored directly as SplitT values.
Test updates for split field rename cpp/tests/sg/rf_test.cu	Classification and regression tests copy DT::Split directly and validate global_nLeft and local_nLeft.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• rapidsai/cuml#8132: Touches related weighted split accounting and child instance-range handling.
• rapidsai/cuml#8257: Modifies the related row-splitting kernel and segmented-scan path.
• rapidsai/cuml#8283: Updates overlapping split range tracking and decision-tree split handling.

Suggested labels: improvement, non-breaking, CUDA/C++

Suggested reviewers: hcho3, aamijar, dantegd, csadorf

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: separating RF split counts for distributed training.
Description check	✅ Passed	The description matches the changeset and explains the split-count separation, test cleanup, and motivation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cpp/src/decisiontree/batched-levelalgo/builder.cuh (1)

111-126: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Avoid truncating global_nLeft to IdxT here
CreateLeafNode() takes IdxT (int in this instantiation), but split.global_nLeft is std::int64_t. The leaf counts at lines 111 and 123 narrow here; if they exceed int, the tree stores the wrong sample count. Widen the node count type or wrap the call with ML::narrow_cast<IdxT>(...).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/src/decisiontree/batched-levelalgo/builder.cuh` around lines 111 - 126,
The leaf-count creation in builder.cuh is narrowing split.global_nLeft into
NodeT::CreateLeafNode, which can truncate counts when IdxT is smaller than
std::int64_t. Update the affected CreateLeafNode calls in the builder logic to
use a safe conversion such as ML::narrow_cast<IdxT> or widen the node count type
used by NodeT so the stored leaf sample counts remain correct. Use the
CreateLeafNode and split.global_nLeft call sites in the batched-levelalgo
builder to locate both conversions.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@cpp/src/decisiontree/batched-levelalgo/builder.cuh`:
- Around line 111-126: The leaf-count creation in builder.cuh is narrowing
split.global_nLeft into NodeT::CreateLeafNode, which can truncate counts when
IdxT is smaller than std::int64_t. Update the affected CreateLeafNode calls in
the builder logic to use a safe conversion such as ML::narrow_cast<IdxT> or
widen the node count type used by NodeT so the stored leaf sample counts remain
correct. Use the CreateLeafNode and split.global_nLeft call sites in the
batched-levelalgo builder to locate both conversions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: dabb035f-d74e-4211-9ae2-aea17e6ab9c9

📥 Commits

Reviewing files that changed from the base of the PR and between 753c0c6 and 51a9949.

📒 Files selected for processing (6)

• cpp/src/decisiontree/batched-levelalgo/builder.cuh
• cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh
• cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh
• cpp/src/decisiontree/batched-levelalgo/objectives.cuh
• cpp/src/decisiontree/batched-levelalgo/split.cuh
• cpp/tests/sg/rf_test.cu

[chyunsu3]

LGTM. Global/local counts are nice abstractions, and I appreciate that we now have a single split type for both host and device. (No more HostSplit)

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

putComment timed out

[jcrist]

/merge