Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

suppress alias warnings with verbosity<0 (fixes #4518) #5253

Merged
merged 18 commits into from
Oct 11, 2022
Merged

suppress alias warnings with verbosity<0 (fixes #4518) #5253

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c5b7e36
set logging level before alias transform
jmoralez May 25, 2022
e99946a
test alias warnings
jmoralez May 30, 2022
36631f8
Merge branch 'master' into alias-warnings
jmoralez May 30, 2022
feb3b86
apply suggestions from review
jmoralez Jun 1, 2022
620c6bd
handle all logs and duplicate parameters
jmoralez Jun 25, 2022
8b36983
merge master
jmoralez Jun 25, 2022
a224553
move setting verbosity to function. add test to check that warns abou…
jmoralez Jun 25, 2022
6a96366
fix tests
jmoralez Jun 25, 2022
88dcf59
use map of vectors
jmoralez Jul 22, 2022
b42781b
merge master
jmoralez Jul 22, 2022
d26cb59
use c_str
jmoralez Jul 22, 2022
96a11fd
remove previous functions
jmoralez Jul 22, 2022
56474ad
warn about config file after setting verbosity
jmoralez Jul 25, 2022
c10f4aa
merge master
jmoralez Jul 31, 2022
eb55c47
rename functions
jmoralez Aug 7, 2022
956f936
update application
jmoralez Aug 7, 2022
e9a7a0f
rename function
jmoralez Aug 16, 2022
e216e43
Merge branch 'master' into alias-warnings
jmoralez Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/io/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ std::unordered_map<std::string, std::string> Config::Str2Map(const char* paramet
for (auto arg : args) {
KV2Map(&params, Common::Trim(arg).c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the following warnings in KV2Map() function are still cannot be suppressed by passing verbosity param because // define verbosity and set logging level happens later in the code

LightGBM/src/io/config.cpp

Lines 28 to 35 in 1cc9f9d

} else {
Log::Warning("%s is set=%s, %s=%s will be ignored. Current value: %s=%s",
key.c_str(), value_search->second.c_str(), key.c_str(), value.c_str(),
key.c_str(), value_search->second.c_str());
}
}
} else {
Log::Warning("Unknown parameter %s", kv);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These warnings come from duplicated parameters, not aliases. I can try to address them as well in this PR if its in the scope.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's related.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second thought on this, maybe we should leave the duplicates warning? I think it helps the user more than being annoying, since you can remove the warnings by removing the duplicate argument or you can confirm that the argument passed through the CLI is overriding the one on the config file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. I believe each warning is helpful, but unfortunately LightGBM users want to completely silence their program (according to the issues they create in the repo).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed by having an unordered_map of vectors where we store all parameters for as many times as they're defined, set the verbosity taking the first values of verbose and verbosity and then de-duplicating with the KeepFirstValues function, where warning level logs are printed but after the verbosity has been set, so these can be disabled as well.

}
// define verbosity and set logging level
int verbosity = Config().verbosity;
jmoralez marked this conversation as resolved.
Show resolved Hide resolved
GetInt(params, "verbose", &verbosity);
GetInt(params, "verbosity", &verbosity);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If params have both verbose and verbosity keys I think we should still emit the following warning for this case

LightGBM/include/LightGBM/config.h

Lines 1145 to 1147 in 1cc9f9d

Log::Warning("%s is set with %s=%s, %s=%s will be ignored. Current value: %s=%s",
alias->second.c_str(), alias_set->second.c_str(), params->at(alias_set->second).c_str(),
pair.first.c_str(), pair.second.c_str(), alias->second.c_str(), params->at(alias_set->second).c_str());

to inform user that verbosity has higher precedence over verbose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done in the KeyAliasTransform function after setting the verbosity and de-duplicating.

jmoralez marked this conversation as resolved.
Show resolved Hide resolved
if (verbosity < 0) {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Fatal);
} else if (verbosity == 0) {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Warning);
} else if (verbosity == 1) {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Info);
} else {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Debug);
}
ParameterAlias::KeyAliasTransform(&params);
return params;
}
Expand Down Expand Up @@ -240,16 +253,6 @@ void Config::Set(const std::unordered_map<std::string, std::string>& params) {
save_binary = true;
}

if (verbosity == 1) {
jmoralez marked this conversation as resolved.
Show resolved Hide resolved
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Info);
} else if (verbosity == 0) {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Warning);
} else if (verbosity >= 2) {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Debug);
} else {
LightGBM::Log::ResetLogLevel(LightGBM::LogLevel::Fatal);
}

// check for conflicts
CheckParamConflict();
}
Expand Down
25 changes: 25 additions & 0 deletions tests/python_package_test/test_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import pickle
import platform
import random
import re
from os import getenv
from pathlib import Path

Expand Down Expand Up @@ -3578,3 +3579,27 @@ def test_boost_from_average_with_single_leaf_trees():
preds = model.predict(X)
mean_preds = np.mean(preds)
assert y.min() <= mean_preds <= y.max()


@pytest.mark.parametrize('verbosity_param', lgb.basic._ConfigAliases.get("verbosity"))
@pytest.mark.parametrize('verbosity', [-1, 0])
jmoralez marked this conversation as resolved.
Show resolved Hide resolved
def test_verbosity_can_suppress_alias_warnings(capsys, verbosity_param, verbosity):
jmoralez marked this conversation as resolved.
Show resolved Hide resolved
X, y = make_synthetic_regression()
ds = lgb.Dataset(X, y)
params = {
'num_leaves': 3,
'subsample': 0.75,
'bagging_fraction': 0.8,
'force_col_wise': True,
verbosity_param: verbosity,
}
lgb.train(params, ds, num_boost_round=1)
expected_msg = (
'[LightGBM] [Warning] bagging_fraction is set=0.8, subsample=0.75 will be ignored. '
'Current value: bagging_fraction=0.8'
)
stdout = capsys.readouterr().out
if verbosity == -1:
assert re.search(r'\[LightGBM\]', stdout) is None
else:
assert expected_msg in stdout
jmoralez marked this conversation as resolved.
Show resolved Hide resolved