[c++] raise a clear error for unknown metric types#7234
[c++] raise a clear error for unknown metric types#7234FranciscoRMendes wants to merge 6 commits into
Conversation
Previously, passing an unrecognised metric name (e.g. "nonsense") to
lgb.train() or lgb.cv() caused a confusing "subscript out of bounds"
error deep inside the R callback machinery, with no indication of what
went wrong.
Root cause: Metric::CreateMetric() returned nullptr for any metric name
it didn't recognise. All call sites treated nullptr as a silent no-op
("continue"), so training proceeded with zero evaluation metrics. In
the R package this produced an empty eval_list, and callbacks that
expected at least one entry crashed with a cryptic subscript error.
Fix: call Log::Fatal("Unknown metric type: %s") before returning
nullptr for any unrecognised type. The only legitimate nullptr path is
the "custom" sentinel value, which ParseMetricAlias produces for
"none" / "na" / "null" / "custom" inputs and which intentionally means
"no built-in metric". That path is preserved unchanged.
The fatal error is raised during Booster initialisation, long before
any callback runs, and propagates as a normal error in Python and R.
… work around pyarrow type-checking issues (lightgbm-org#7236)
There was a problem hiding this comment.
Thanks for the help on this long-standing usability issue!
I'm supportive of raising an error in this situation, as @guolinke was back in 2020: #3481 (comment)
The R and Python tests look pretty good, thanks very much for following the existing patterns in the code base and keeping those tests focused and lightweight.
Metric::CreateMetric() returned nullptr for any unrecognised metric name.
Please use commit-anchored links (GitHub docs) when referring to code specifics like this. In this case:
LightGBM/src/metric/metric.cpp
Line 139 in 4472f39
It helps reviewers to quickly navigate to the code you're referring to, and to see a preview of it rendered in the GitHub UI to avoid needing to click at all.
In #3481 (comment), @guolinke wrote:
The same strategy could be adapted to objective functions
Did you look into that at all? If not, would you be interested in doing that in a follow-up PR?
If so, the PR description should be changed from "Closes" to "Contributes to"... that issue is tracking doing this for both metrics and objective functions.
Test plan
The PR description is clear and thorough, thanks for that. I've seen this exact structure many times recently (GitHub search), and know it's the type of thing that's generated by LLMs.
Use the tools you want for coding, but when you contribute here, please:
- write explanatory text (comments, PR description, issue description) yourself... I'm happy to talk to you, I do not want to talk to an LLM through you
- be ready to explain your thought process and the meaning of the submitted code yourself ("I'm not sure, this is what the LLM said" will not be acceptable)
- error message now uses single quotes around the metric name and includes a hint about metric='none' for disabling built-in metrics - drop 3-value parametrize on unknown metric test; add separate tests for sentinel string values and Python None
ef20f10 to
10f2574
Compare
|
I'll take a closer look at these PRs tonight, but just a quick note... you don't need to rebase and force-push in this repo. We squash all commits to 1 on merge. In fact, we'd generally prefer you didn't force-push once PR review has started. That makes it easier for reviewers to see what you changed in response to the conversation. Work however you're comfortable working, just letting you know you don't need to worry too much about the individual commit history within a PR because we squash. |
Summary
When a user passes an unknown metric name to lgb.train() or lgb.cv(), LightGBM previously silently proceeded with no evaluation metrics. In the R package this caused a confusing crash inside the callback machinery (subscript out of bound) rather than a clear
error pointing to the bad metric name.
The fix is in Metric::CreateMetric() (src/metric/metric.cpp:L139 (
LightGBM/src/metric/metric.cpp
Line 139 in 4472f39
The only exception is the "custom" sentinel that ParseMetricAlias produces for "none" / "na" / "null" / "custom" — those are intentional no-ops and still return nullptr.
Tests
Contributes to #3481