Skip to content

[cuda] implement cost-effective gradient boosting on the CUDA tree learner#18

Open
maxwbuckley wants to merge 1 commit into
BelixRogner:masterfrom
maxwbuckley:cuda/cegb-error
Open

[cuda] implement cost-effective gradient boosting on the CUDA tree learner#18
maxwbuckley wants to merge 1 commit into
BelixRogner:masterfrom
maxwbuckley:cuda/cegb-error

Conversation

@maxwbuckley

@maxwbuckley maxwbuckley commented May 31, 2026

Copy link
Copy Markdown
Collaborator

Problem: the delta

When training with device_type="cuda", the cost-effective gradient boosting parameters (cegb_tradeoff, cegb_penalty_split, cegb_penalty_feature_coupled, cegb_penalty_feature_lazy) were silently ignored. CEGB lives only in the CPU serial learner (CostEfficientGradientBoosting), which subtracts a per-split cost from each candidate's gain; the CUDA kernels had no penalty plumbing, and the CUDA learner had no "stop when best penalized gain ≤ 0" condition.

Measured delta before this PR (400×6 synthetic data, learning_rate=0.1, 5 rounds, gpu_use_dp=true):

case per-tree leaves CPU per-tree leaves CUDA max pred delta
cegb_penalty_split=0.1 [8, 10, 4, 6, 6] [31, 31, 31, 31, 31] 0.31
cegb_penalty_split=1.0 [1] (growth stops) [31, 31, 31, 31, 31] 0.91
cegb_penalty_split=5.0 [1] (growth stops) [31, 31, 31, 31, 31] 0.91
cegb_tradeoff=0.5, penalty_split=1.0 [2] [31, 31, 31, 31, 31] 0.83

CPU prunes aggressively (down to a stump when the penalty exceeds all gains); CUDA grew full 31-leaf trees regardless — the penalties had zero effect.

Fix

Implements CEGB in the CUDA best-split finder:

  • Split + coupled penalties: the cost delta tradeoff · penalty_split · num_data_in_leaf + tradeoff · coupled[real_fidx] (coupled term only while a feature is unused anywhere in the model) is subtracted from each task's finalized gain in the dispatch kernels — before cross-feature/cross-leaf comparison, exactly where CPU applies new_split.gain -= cegb_->DeltaGain(...).
  • Feature-usage tracking: per-task coupled penalties live in a device array; the host mirrors CPU's is_feature_used_in_split_ (accumulated over the whole model, not per tree) and zeroes a feature's penalty entries the first time it's used in any split.
  • Stop condition: FindBestFromAllSplitsKernel now requires gain > 0 to select a split, mirroring CPU's if (best_leaf_SplitInfo.gain <= 0.0) break. Without CEGB this is a no-op (valid splits always have positive gain); with CEGB the penalty can push every gain negative and growth must stop — this was the missing piece that let CUDA keep splitting at gain ≤ 0.
  • cegb_penalty_feature_lazy: needs per-(row, feature) cost tracking (a device bitset + per-leaf scan); not implemented — now rejected with a clear Log::Fatal instead of being silently ignored.

The first commit's blanket guard is replaced: split and coupled penalties work on CUDA; only the lazy penalty is still rejected.

Result: the delta after the fix

Same measurement, post-fix:

case per-tree leaves CPU per-tree leaves CUDA match max pred delta
cegb_penalty_split=0.1 [8, 10, 4, 6, 6] [8, 10, 4, 6, 6] 0.0
cegb_penalty_split=1.0 [1] [1] 0.0
cegb_penalty_split=5.0 [1] [1] 0.0
coupled=[5,5,5,5,5,5] [31, ...] [31, ...] 2.2e-16
coupled=[0.1, 0.1, 5, 5, 5, 5] [31, ...] [31, ...] 2.2e-16
cegb_tradeoff=0.5, penalty_split=1.0 [2] [2] 0.0

Identical tree shapes and feature selection on every case; predictions match bit-for-bit (≤ 1 fp64 ULP).

Tests (in test_dual.py, gated on TASK=cuda)

  • test_cuda_cegb_matches_cpu — 6 parametrized CEGB configs, asserting per-tree leaf counts, feature sets, and predictions (atol=1e-10) all match CPU. All fail on the unpatched build (CUDA grows 31-leaf trees); all pass with the fix.
  • test_cuda_cegb_lazy_penalty_raises — the unimplemented lazy penalty raises a clear error.

Full test_dual.py suite (61 tests) passes.

Known limitation

CPU's UpdateLeafBestSplits retroactively re-evaluates cached splits in other leaves when a feature first becomes used (the coupled penalty disappears for those cached candidates). CUDA caches one best split per leaf and cannot recover a runner-up without a per-(leaf, feature) buffer; in adversarial coupled-penalty configurations this second-order effect could pick a different (still valid) split order. Not observed in any test configuration; documented for completeness.

🤖 Generated with Claude Code

@maxwbuckley maxwbuckley changed the title [cuda] reject cost-effective gradient boosting instead of silently ignoring it [cuda] implement cost-effective gradient boosting on the CUDA tree learner Jun 1, 2026
@maxwbuckley maxwbuckley marked this pull request as ready for review June 1, 2026 20:34
@BelixRogner

Copy link
Copy Markdown
Owner

Thank you, Max — and thank you, Claude Code (independently 🙂). Verdict: SOLID, no blocking bug for the supported/tested usage.

Reviewed against CostEfficientGradientBoosting::DeltaGain and the CPU subtraction site in serial_tree_learner.cpp:

  • Cost-delta tradeoff·penalty_split·num_data_in_leaf + tradeoff·coupled[real_fidx] matches CPU exactly, subtracted on the finalized per-task gain before the cross-feature/cross-leaf reductions — same place as new_split.gain -= cegb_->DeltaGain(...).
  • Feature-usage tracking is genuinely model-global (only assigned in SetCEGB/Init, deliberately not reset in BeforeTrain), matching CPU's if (!init_) semantics.
  • The gain > 0 stop condition is strict and a true no-op without CEGB — verified valid splits always carry strictly-positive stored gain (current_gain > min_gain_shift), so it can't drop a legitimate split even at min_gain_to_split=0.
  • cegb_penalty_feature_lazy → clear Log::Fatal. num_data_in_leaf is the correct per-leaf count at evaluation time.

Two non-blocking notes (uncommon paths, worth a docstring or follow-up, not a merge blocker):

  • reset_parameter mid-training won't change cegb_* on CUDA — penalties are snapshotted at Init; ResetConfig doesn't re-run SetCEGB. CPU reads them live.
  • The kernels use global_num_data_in_*_leaf; identical to local for single-GPU (your tested path), but would differ from CPU per-row penalty scale under multi-GPU.

Your documented "retroactive re-eval" limitation is real and honestly stated — the tests just don't engineer a case that triggers a promotion. Fine to leave as-is.

Merge blocker is only the required ruff format check + a git merge master (these five overlap). macOS red = dask socket flake.

P.S. — you proved bit-for-bit leaf-count and feature-set parity across 6 CEGB configs and then shipped unformatted test code. 😄 The penalty for skipping ruff format is, fittingly, cost-effective: one command.

@BelixRogner

Copy link
Copy Markdown
Owner

Thank you, Max — and thank you, Claude Code (independently 🙂). Rebase nudge 👉

This one reviewed out SOLID — it's only CONFLICTING because master moved under it (forced-splits #17 landed, plus a stack of best-split-finder / data-partition merges: #21#27). A rebase onto current master should clear it cleanly.

You have write access now (accept the repo invite if you haven't), so once it's rebased and green you can merge it yourself. 🚀

@maxwbuckley

Copy link
Copy Markdown
Collaborator Author

Rebased onto current master. 👋

Please re-review — there is one behavioural change I had to make, called out below.

What changed. Rebuilt as master + this one commit (the old branch carried ~28 commits already landed under different SHAs). Now 6 files, 1 commit.

⚠️ The one thing to look at: MarkFeatureUsedInSplit() moved into ApplySplit().

The original PR called it inline in Train(), right after tree->Split(...). Since then your #17 extracted that whole block into ApplySplit() — and there are now two call sites: the normal best-split path and the forced-split path.

CEGB charges cegb_penalty_feature_coupled the first time a feature is used in the tree, so every path that commits a split has to mark it. Leaving the call at the old site would have marked features on normal splits but silently not on forced splits, so a feature first used by a forced split would get charged the coupled penalty again later. I moved the call inside ApplySplit() so both paths mark uniformly. It reads leaf_best_split_feature_[leaf_index], which the forced-split path sets just before calling, and it's a no-op unless CEGB is configured.

This is a change from what you reviewed, and it's the bit I'd most like a second opinion on.

Also: the conflict in src/io/config.cpp was an artifact of the old branch's "reject CEGB" commit, which I dropped (it was superseded by this PR's own implementation, and master never had the rejection). What lands is this PR's narrower guard — CEGB is supported on CUDA except cegb_penalty_feature_lazy, which still raises.

In cuda_single_gpu_tree_learner.cpp the init conflict was purely additive: master's SetHistogramEvents() and this PR's SetCEGB() both kept.

Validation (RTX 5090, CUDA 13.2, built from source): 7 CEGB tests pass (including the lazy-penalty rejection); full test_dual.py = 126 passed, 1 skipped — the forced-split tests pass with the relocated marking. cpplint / ruff clean.

Rebased, re-integrated and validated with Claude Code; real GPU runs, not CI.

…arner

Implements CEGB (cegb_tradeoff, cegb_penalty_split,
cegb_penalty_feature_coupled) in the CUDA best-split finder, matching the
CPU CostEfficientGradientBoosting behavior:

- the per-split cost delta (tradeoff * penalty_split * num_data_in_leaf,
  plus tradeoff * coupled[real_fidx] while a feature is unused in the model)
  is subtracted from each task's finalized gain in the dispatch kernels,
  before cross-feature / cross-leaf comparison - exactly where the CPU
  serial learner applies new_split.gain -= cegb_->DeltaGain(...)
- per-task coupled penalties live in a device array; the host tracks which
  features have been used in any split (mirroring is_feature_used_in_split_,
  accumulated over the whole model) and zeroes a feature's penalty entries
  the first time it is used
- FindBestFromAllSplitsKernel now requires gain > 0 to select a split,
  mirroring the CPU stop condition (best gain <= 0 -> stop). Without CEGB
  this is a no-op since valid splits always have positive gain; with CEGB
  the penalty can push all gains negative and growth must stop
- cegb_penalty_feature_lazy (per-row-per-feature cost tracking) is not
  implemented and now fails fast with a clear error

This replaces the previous blanket Log::Fatal guard: split and coupled
penalties now work on CUDA; only the lazy penalty is still rejected.

Before this change CUDA silently ignored CEGB: with cegb_penalty_split=1.0
CPU stopped at a single root leaf while CUDA grew 31 leaves in every tree
(prediction divergence 0.91). After this change per-tree leaf counts and
selected features match CPU exactly and predictions agree at FP epsilon
(<= 2.2e-16) on all test configurations.

Adds parametrized parity tests (6 CEGB configs) plus a lazy-penalty
rejection test to test_dual.py.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 04a3cf7)
@maxwbuckley

Copy link
Copy Markdown
Collaborator Author

Re-rebased onto current master (the hybrid-growth merges). This one changed materially again — please read before merging.

CEGB is incompatible with hybrid level-batched growth

Validating on the new master caught this: with hybrid growth on (the default), CUDA ignored CEGB entirely. CPU grew trees of 8 / 10 / 4 / 6 / 6 leaves (and a 1-leaf stump in one config); CUDA grew 31 leaves every time.

Root cause is structural, not a plumbing bug. CEGB is order-dependent: cegb_penalty_feature_coupled is charged the first time a feature is used, so a split's gain depends on which splits were already applied. Level-batched growth scores a whole level before applying any of it, so the coupled penalty would be charged to every leaf in the level instead of just the first. Even after I wired the penalty into the batched kernels, 5 of 6 cases still diverged — the order dependence can't be plumbed away.

Fix: fall back to the classic leaf-wise loop when CEGB is enabled, exactly the way the batched path is already gated off for categorical features:

if (CostEfficientGradientBoosting::IsEnable(config_)) {
  use_hybrid_growth_ = false;
}

Confirmed empirically: EXABOOST_HYBRID_GROWTH=0 makes all 6 CEGB parity tests pass on unmodified code. With the gate, they pass by default.

Worth knowing independently of this PR: any order-dependent feature is unsafe on the batched path. I gated monotone constraints off the same way in #16.

Also

  • MarkFeatureUsedInSplit() lives inside ApplySplit() so the forced-split path marks too (as before).
  • The config.cpp conflict was the old "reject CEGB" commit, which I dropped; what lands is the narrower guard (CEGB supported except cegb_penalty_feature_lazy).

Validation

RTX 5090, CUDA 13.2: 7 CEGB tests pass; test_dual.py = 114 passed, 12 failed — all 12 are a pre-existing master regression, not this PR (see my note on #35). cpplint clean.

Validated with Claude Code on real GPU runs; CI has no GPU and never exercises any of this.

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.

2 participants