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

jmoralez · 2022-05-30T20:39:08Z

Sets the logging level before resolving the parameter aliases so that the warnings related to them can be suppressed using the verbosity parameter.

Fixes #4518.

jameslamb

Thanks for doing this! Like I mentioned in #4518 (comment), I totally support this change. But I'd like to hear @StrikerRUS's perspective on this one as well before this is merged.

Until then, please see a few small suggestions below.

tests/python_package_test/test_engine.py

src/io/config.cpp

tests/python_package_test/test_engine.py

jameslamb

Changes look good to me, thanks! Since Config is so central to the codebase, I'd like one other maintainer to review and approve before we merge this.

src/io/config.cpp

StrikerRUS

Thanks for this PR!
I'm sorry, config initialization routine is still not clear enough for me. However, I left some comments that may help to spot uncovered by this PR cases.

StrikerRUS · 2022-06-05T19:19:46Z

src/io/config.cpp

+  GetInt(params, "verbose", &verbosity);
+  GetInt(params, "verbosity", &verbosity);


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.

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

src/io/config.cpp

StrikerRUS · 2022-06-05T19:37:22Z

src/io/config.cpp

@@ -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());


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);

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

I believe it's related.

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.

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).

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.

tests/python_package_test/test_engine.py

…t verbosity param are shown

jmoralez · 2022-06-25T22:54:54Z

I've changed the approach, now:

Every parameter is saved to an unordered_multimap which can have several values for the same key, this is so we can see if there are any duplicate parameters after parsing them.
Once we have all the parameters in the multimap we check for verbose and verbosity and set the logging level.
We then move the multimap to a regular map and warn about duplicate arguments. This step takes a random one if two or more are provided (we can discuss this).
Once the parameters are in a map we check for the aliases as before.

This of course uses more memory now but I couldn't think of a better way to handle the possible duplicate parameters. I'm open to adapt this to a better idea.

jmoralez · 2022-06-25T23:36:11Z

There are some issues right now with this because it prints the warnings several times and there seems to be a weird interaction with the dataset both in the python and R-packages where if you don't re-construct the dataset, verbosity -1 doesn't remove the alias warnings. I have to look into that.

The CLI works fine though 😄

jmoralez · 2022-06-28T01:54:02Z

Seems like the duplicate warnings about aliases is on master as well. I'll open an issue.

guolinke

Thank you!

src/application/application.cpp

StrikerRUS · 2022-07-30T21:42:48Z

Could you please resolve merge conflicts?

StrikerRUS

Thanks a lot for this PR!
I don't have any other additional comments except a one below about the function naming.

But I think I'm not qualified to formally approve these mostly cpp changes.
✔️

src/io/config.cpp

guolinke

Looks good to me, except a minor naming

StrikerRUS · 2022-08-27T23:48:34Z

Can we merge this PR?

jameslamb · 2022-09-03T04:58:11Z

Seems like the duplicate warnings about aliases is on master as well. I'll open an issue.

did this happen? I couldn't find an issue like that from search and don't see one linked here.

Can we merge this PR?

Based on what I see in the diff for unit tests, I think this can be merged. It does seem like the warnings described in #3641 (comment) (which led to #4518) can now be suppressed with verbosity.

But I'll defer to @jmoralez . To be honest, there have been significant changes since I first approved and it's difficult for me to understand the current state of this PR a bit.

For example, the conversation in #5253 (comment) is unresolved.

jmoralez · 2022-09-03T05:35:02Z

did this happen?

Yes, I opened #5332.

The duplicate parameter warnings were also addressed in this PR. The KeepFirstValues function keeps the first value defined for each parameter and warns about the rest (if there are any). This allows to override parameters defined in a configuration file through arguments in the CLI, or if defined twice in the parameter list in the R-package, the first one takes precedence and a warning is printed for the second one.

My opinion is that this is ready to be merged, but of course I'm biased haha.

jameslamb

Yes, I opened #5332.

ah ok, thanks for pointing that out! I just got unlucky with the search terms I chose 😅

If that issue had mentioned this PR with #5253 or if a comment had been posted on this PR saying "created #5332", it would have been easier for me to find.

The duplicate parameter warnings were also addressed in this PR

Sorry, I'm still confused...do you mean specifically that as of this PR being merged, we could also close #5332?

If so, please:

add "fixes alias warnings can print several times #5332" to the description
add one more test confirming that the duplicate warnings are no longer raised

jmoralez · 2022-09-05T14:25:29Z

do you mean specifically that as of this PR being merged, we could also close #5332?

No, I meant that this was originally only going to remove warnings for parameter aliases, i.e. you set bagging_fraction and subsample, but in #5253 (comment) it was suggested that this also addressed warnings for duplicate parameters, i.e. you set bagging_fraction=0.7 and bagging_fraction=0.8, so that conversation was resolved because it was also addressed.

The duplicate prints (#5332) is not solved yet.

jameslamb

Thanks for this and sorry for taking so long to come back and re-review! I understand now what his PR does and doesn't fix.

Let's merge this...it's only changing internal details, so easy to revise any of these design decisions in the future if we decide a different pattern is preferable for changing other logging-related stuff.

Awesome contribution, @jmoralez ! People have been asking for this for a long time 😬

github-actions · 2023-08-19T03:04:28Z

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

jmoralez added 3 commits May 25, 2022 16:50

set logging level before alias transform

c5b7e36

test alias warnings

e99946a

Merge branch 'master' into alias-warnings

36631f8

jmoralez requested review from guolinke, shiyu1994, StrikerRUS and jameslamb as code owners May 30, 2022 20:39

jameslamb requested changes May 31, 2022

View reviewed changes

tests/python_package_test/test_engine.py Show resolved Hide resolved

src/io/config.cpp Outdated Show resolved Hide resolved

tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved

jameslamb added the maintenance label May 31, 2022

apply suggestions from review

feb3b86

jmoralez changed the title ~~suppress alias warnings with verbosity=-1~~ suppress alias warnings with verbosity<0 Jun 1, 2022

jmoralez changed the title ~~suppress alias warnings with verbosity<0~~ suppress alias warnings with verbosity<0 (fixes #4518) Jun 1, 2022

jameslamb approved these changes Jun 4, 2022

View reviewed changes

src/io/config.cpp Show resolved Hide resolved

StrikerRUS reviewed Jun 5, 2022

View reviewed changes

jmoralez added the in progress label Jun 23, 2022

jmoralez added 3 commits June 25, 2022 17:23

handle all logs and duplicate parameters

620c6bd

merge master

8b36983

move setting verbosity to function. add test to check that warns abou…

a224553

…t verbosity param are shown

fix tests

6a96366

jmoralez added 4 commits July 21, 2022 23:42

use map of vectors

88dcf59

merge master

b42781b

use c_str

d26cb59

remove previous functions

96a11fd

guolinke approved these changes Jul 23, 2022

View reviewed changes

StrikerRUS reviewed Jul 24, 2022

View reviewed changes

src/application/application.cpp Outdated Show resolved Hide resolved

warn about config file after setting verbosity

56474ad

StrikerRUS removed the in progress label Jul 30, 2022

merge master

c10f4aa

StrikerRUS reviewed Aug 2, 2022

View reviewed changes

src/io/config.cpp Outdated Show resolved Hide resolved

jmoralez added 2 commits August 6, 2022 19:49

rename functions

eb55c47

update application

956f936

guolinke reviewed Aug 16, 2022

View reviewed changes

src/io/config.cpp Outdated Show resolved Hide resolved

guolinke approved these changes Aug 16, 2022

View reviewed changes

rename function

e9a7a0f

jameslamb self-requested a review September 5, 2022 04:20

jameslamb requested changes Sep 5, 2022

View reviewed changes

Merge branch 'master' into alias-warnings

e216e43

jameslamb approved these changes Oct 11, 2022

View reviewed changes

jameslamb merged commit 4642712 into microsoft:master Oct 11, 2022

jmoralez deleted the alias-warnings branch October 11, 2022 04:17

github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jmoralez commented May 30, 2022

jameslamb left a comment •

edited

Loading

jameslamb left a comment

StrikerRUS left a comment

StrikerRUS Jun 5, 2022

jmoralez Sep 3, 2022

StrikerRUS Jun 5, 2022

jmoralez Jun 9, 2022

StrikerRUS Jun 12, 2022

jmoralez Jun 30, 2022

StrikerRUS Jul 3, 2022

jmoralez Sep 3, 2022

jmoralez commented Jun 25, 2022 •

edited

Loading

jmoralez commented Jun 25, 2022 •

edited

Loading

jmoralez commented Jun 28, 2022

guolinke left a comment

StrikerRUS commented Jul 30, 2022

StrikerRUS left a comment

guolinke left a comment

StrikerRUS commented Aug 27, 2022

jameslamb commented Sep 3, 2022

jmoralez commented Sep 3, 2022

jameslamb left a comment •

edited

Loading

jmoralez commented Sep 5, 2022

jameslamb left a comment

github-actions bot commented Aug 19, 2023

		GetInt(params, "verbose", &verbosity);
		GetInt(params, "verbosity", &verbosity);

	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());

		@@ -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());

	} 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);

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

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

Conversation

jmoralez commented May 30, 2022

jameslamb left a comment • edited Loading

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmoralez commented Jun 25, 2022 • edited Loading

jmoralez commented Jun 25, 2022 • edited Loading

jmoralez commented Jun 28, 2022

guolinke left a comment

Choose a reason for hiding this comment

StrikerRUS commented Jul 30, 2022

StrikerRUS left a comment

Choose a reason for hiding this comment

guolinke left a comment

Choose a reason for hiding this comment

StrikerRUS commented Aug 27, 2022

jameslamb commented Sep 3, 2022

jmoralez commented Sep 3, 2022

jameslamb left a comment • edited Loading

Choose a reason for hiding this comment

jmoralez commented Sep 5, 2022

jameslamb left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 19, 2023

jameslamb left a comment •

edited

Loading

jmoralez commented Jun 25, 2022 •

edited

Loading

jmoralez commented Jun 25, 2022 •

edited

Loading

jameslamb left a comment •

edited

Loading