[dask] Aggregate distributed validation metrics, fix dask early stopping#7290
[dask] Aggregate distributed validation metrics, fix dask early stopping#7290szjunma wants to merge 17 commits into
Conversation
jameslamb
left a comment
There was a problem hiding this comment.
Thanks for your interest in LightGBM. I'm open to you exploring this (I do see how it could be valuable) but there is much to figure out for this to work well.
keep ordering/query-dependent metrics like AUC, average precision, AUC-Mu, NDCG, and MAP out of scope
Thanks for calling this out. Please remove "fixes" from the PR description... this PR in its current state is not enough to close the issue. It doesn't cover:
- the metrics you listed
- custom metrics implemented as Python functions
I think being inconsistent there will be confusing for users. Without looking at the code how would they know whether evals_result contains a global or a local-to-one-worker metric?
This should at a minimum be documented, maybe somewhere in the constructor docstrings for the Dask estimators.
## Test Plan
Did you test these changes yourself? It looks to me like you might have pointed an LLM at #6963, said "make a PR", and had it just commit and push whatever it came up with without review. I see strong evidence of that... common LLM-generated-PR template in the description, confusing choices in the code, and overly-verbose tests.
If so, please don't do that! If you're interested in contributing to LightGBM I'm happy to work with you, but I refuse to talk to an LLM through you.
If you'd like to continue, please:
- address all the requests in my first round of comments
- re-write the PR description in your own words
- from this point forward, write all review discussion in your own words (no copying in LLM responses)
- review every line of code yourself before committing (no automatically pushing LLM-generated code to the PR branch)
|
It looks to me like this change also breaks some of the Dask tests. These look relevant: Before pushing your next round of commits, please try testing locally. cmake -B build -S .
cmake --build build --target _lightgbm -j4
sh build-python.sh -install --precompile
pytest tests/python_package_test/test_dask.py |
|
@jameslamb Thanks for the review. I do plan to work on the dask early stopping issue - this PR is the first part of it. I have put the PR to draft and will continue working on addressing your comments + other improvements. |
|
Oh good to know! We'd be grateful for the help on the Dask integration. Sure, you're welcome to work in draft here for a bit and |
42f36f4 to
d057a03
Compare
|
@jameslamb This PR is ready for a re-review. The changes can be grouped into 4 parts:
Since the last review, I've also added docstring documentation about the aggregation limitations, simplified the tests, and fixed CI failures. |
f9749d9 to
705c4e4
Compare
7ddad23 to
07e17ec
Compare
716c5f9 to
0094d8c
Compare
0094d8c to
13b97b5
Compare
|
Hey @jameslamb, circling back on this — the scope has grown a bit since the last review. It now also covers non-additive metrics, which means all built-in metrics are supported and early stopping in Dask is working (without extra effort). Ready for another look whenever you have time. |
193cd78 to
0ece1d2
Compare
…parameter The parameter generator requires `// desc =` annotations for all parameters in config.h. The new parameter only had a plain comment, causing a KeyError in CI's regenerate-parameters hook.
…aggregation unconditional
This reverts commit 7ddad23.
Add C++ distributed aggregation for the 5 remaining non-additive metrics: - AUC, AveragePrecision, auc_mu: collect (label, score, weight) from all workers via variable-size Network::Allgather (new gather_eval_data.hpp), then compute metric on the consolidated global data - NDCG, MAP: sync per-query weighted sums via GlobalSyncUpBySum, divide after reduction (queries are never split across workers) Zero-row workers participate in collectives with zero/empty contributions to keep Network::Allgather and GlobalSyncUpBySum balanced. Python-side: add all metrics to _AGGREGATED_DISTRIBUTED_EVAL_METRICS, remove ranker/eval_group exclusion from _eval_metrics_are_distributed_aggregated, add eval_group handling in _train_part and eval_group padding in _train, remove leftover eval-data-sparsity warning (no longer applicable). Tests: parametrized validation for AUC, AP, NDCG, MAP; empty-eval-worker tests for AUC, AP, MAP; auc_mu test marked xfail (pending C++ debug).
0ece1d2 to
c6a4ccc
Compare
Summary
This PR adds support to distributed evaluation metrics so every worker sees the globally-aggregated result, which fixes two interrelated problems:
After this PR, all workers see the same global metric and agree on when to stop.
One design choice to make is how to handle workers that receive no eval chunk. In this PR, workers with no local rows for a validation set still participate in evaluation: they register an empty local validation dataset and contribute zero local numerator / denominator to the distributed metric reduction. Because workers with an empty local eval set still enter the same aggregation calls, the distributed collective stays synchronized and no worker hangs waiting for peers that never participate.
What changed
C++ metric aggregation
Python / Dask
Tests
Known out of scope
Custom Python eval callables still see per-worker local values.