Skip to content

[cuda] match CPU's per-group min_data_per_group semantics in categorical splits#35

Open
maxwbuckley wants to merge 1 commit into
BelixRogner:masterfrom
maxwbuckley:cuda/min-data-per-group-parity
Open

[cuda] match CPU's per-group min_data_per_group semantics in categorical splits#35
maxwbuckley wants to merge 1 commit into
BelixRogner:masterfrom
maxwbuckley:cuda/min-data-per-group-parity

Conversation

@maxwbuckley

Copy link
Copy Markdown
Collaborator

Found while rebasing the open CUDA PRs onto current master: CUDA and CPU disagree on categorical splits at the default min_data_per_group=100. This is a pre-existing bug on master, not something any of the open PRs introduced.

The bug

CPU (FeatureHistogram::FindBestThresholdCategoricalInner) treats min_data_per_group as a minimum on the group — the run of sorted categories accumulated since the last accepted threshold. It tracks cnt_cur_group, skips a threshold whose group is still too small, and resets the counter each time a threshold is accepted. Acceptance is therefore sequential: it depends on which earlier thresholds were accepted.

CUDA applied it as an independent per-side predicate instead:

left_count >= min_data_per_group && right_count >= min_data_per_group

Different rule. The two agree when min_data_per_group sits far from the per-category row counts — which is why this went unnoticed — but when it lands near them, CUDA accepts a different set of candidate thresholds, picks a different categorical split, and grows a different tree.

Repro

400 rows, 12 categories (~33 rows each), one boosting round:

min_data_per_group 1 5 20 33 50 100 200
max|CPU − CUDA| 0.0 0.0 0.0 5.2e-2 0.0 6.1e-2 0.0

The default is 100, so this fires on default settings.

Fix

CategoricalThresholdAcceptedByCPU() replays CPU's sequential scan for a given threshold index and returns whether CPU would have evaluated a split there. The scan is O(i) with i < max_cat_threshold (default 32), so replaying it per thread is cheaper than materialising a mask in shared memory — and it needs no extra storage:

  • the shared-memory kernel republishes its prefix hessians into shared_value_buffer, which is dead after the sort;
  • the global-memory kernel already has them in hist_hess_buffer_ptr.

Applied to both categorical kernels, in both scan directions.

Tests

test_cuda_min_data_per_group_categorical_matches_cpu asserts bit-exact CPU/CUDA agreement across min_data_per_group ∈ {1, 5, 20, 33, 50, 100, 200}. It fails on the pre-fix code at 33 and 100.

Validated on an RTX 5090 (CUDA 13.2): full test_dual.py = 126 passed, 1 skipped; test_engine.py CUDA/categorical subset green. cpplint clean with the repo's filters.

Note

This unblocks #10 — its own regression test (test_cuda_bitonic_argsort_1024_with_distinct_scores_matches_cpu) exercises the categorical path and was failing on current master for this reason, not because of anything in #10. I confirmed the failure is bit-identical (0.06108053979861565) with #10's argsort fix reverted, and vanishes entirely once this lands.

🤖 Generated with Claude Code

…cal splits

CPU (FeatureHistogram::FindBestThresholdCategoricalInner) treats
min_data_per_group as a minimum on the *group* -- the run of sorted
categories accumulated since the last accepted threshold. It tracks
cnt_cur_group, `continue`s past a threshold whose group is still too
small, and resets the counter to zero each time a threshold is accepted,
so acceptance depends on which earlier thresholds were accepted.

CUDA applied it as an independent per-side predicate instead:

    left_count >= min_data_per_group && right_count >= min_data_per_group

That is a different rule. The two happen to agree when min_data_per_group
sits far from the per-category row counts, which is why this went
unnoticed -- but whenever it lands near them, CUDA accepts a different set
of candidate thresholds than CPU, picks a different categorical split, and
grows a different tree.

Repro (400 rows, 12 categories ~= 33 rows each, one boosting round):

    min_data_per_group |  1     5     20    33      50    100     200
    max|CPU - CUDA|    | 0.0   0.0   0.0   5.2e-2  0.0   6.1e-2  0.0

The default is 100, so this fired on default settings.

Fix: CategoricalThresholdAcceptedByCPU() replays CPU's sequential scan for
a given threshold index and returns whether CPU would have evaluated a
split there. The scan is O(i) with i < max_cat_threshold (default 32), so
replaying it per thread is cheaper than materialising a mask in shared
memory, and it needs no extra storage: the shared-memory kernel republishes
its prefix hessians into shared_value_buffer (dead after the sort), and the
global-memory kernel already has them in hist_hess_buffer_ptr.

Applied to both categorical kernels (shared-memory and global-memory) in
both scan directions.

Adds test_cuda_min_data_per_group_categorical_matches_cpu, which asserts
bit-exact CPU/CUDA agreement across min_data_per_group in
{1, 5, 20, 33, 50, 100, 200}. Fails on the pre-fix code at 33 and 100.

Full test_dual.py suite: 126 passed, 1 skipped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@maxwbuckley

Copy link
Copy Markdown
Collaborator Author

Rebased onto current master. Also — heads-up on a separate master regression I hit while validating (issues are disabled on the repo, so parking it here since it's the baseline for every open PR).

master is red on its own parity suite

Current master (dd55ece) fails 12 of its own CPU/CUDA parity tests on a GPU. CI doesn't catch it because CI has no GPU.

TASK=cuda pytest tests/python_package_test/test_dual.py
=> 12 failed, 107 passed, 1 skipped

All 12 are test_cuda_init_score_matches_cpu[*-quantile-0.5] / [*-quantile-0.7], failing with a hard error rather than a numeric mismatch:

LightGBMError: [CUDA] invalid argument   src/io/cuda/cuda_column_data.cu:27

That line is CUDASUCCESS_OR_FATAL(cudaGetLastError()) inside WarmupCUDAKernelModule(), added in 2476296 "move one-time first-Train() costs out of the training wall clock".

The warmup is the messenger, not the bug

The same test passes in isolation:

run result
...::test_cuda_init_score_matches_cpu[5-quantile-0.5] alone 1 passed
...::test_cuda_init_score_matches_cpu (whole set) 12 failed, 12 passed

It only fails when earlier tests ran first in the same process. cudaGetLastError() returns and clears the last error from any prior CUDA call — so the warmup is surfacing a pre-existing sticky cudaErrorInvalidValue left by earlier work (most likely the quantile/percentile init-score path). Previously silent; now fatal, and attributed to the wrong place.

Two separate things worth fixing:

  1. The latent CUDA error itself — real, and previously being swallowed. The new check did us a favour by exposing it.
  2. The check's placement — clearing before checking would stop it misattributing unrelated failures (though that would re-hide [cuda] fix CUDA quantized training to match CPU #1, so I'd chase [cuda] fix CUDA quantized training to match CPU #1 first).

Why it matters for review

This is the baseline for every open PR: all 8 of my rebased branches show exactly these 12 failures and nothing else. When I say a PR has "no failures beyond master's 12", that's what I mean — none of them add a failure.


As for this PR itself (the min_data_per_group per-group semantics fix): unchanged, rebased cleanly, still 114 passed / 12 master-failures, and it remains the prerequisite for #10.

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.

1 participant