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.

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

[RFC][python] deprecate silent and standalone verbose args. Prefer global verbose param #4577

Merged
merged 3 commits into from
Sep 4, 2021

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Aug 30, 2021

Proposing this PR to include in #4310

I also think this release could be a good opportunity for maintainers to think carefully about what breaking changes might be made (in addition to #3234) in a 4.0.0 release, and to add deprecation warnings for them in the 3.3.0 release.

Having multiple params and arguments that control verbosity is quite confusing, see #1157 (comment). I believe leaving only verbose keyword argument, that is described in our general parameters guide and which one that a lot of users is used to simply set to -1, will simplify user experience and decrease the maintainance burden for us.

Comment on lines 3300 to 3301
if verbose:
_log_info(f'Finished loading model, total used {int(out_num_iterations.value)} iterations')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this one, I don't think one log line deserves a standalone argument. Users are able to get number of iterations after constructing a Booster if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree! (sorry I missed commenting on this in my previous review)

@jameslamb
Copy link
Collaborator

I support this change, good idea!

But I have a mild preference for these deprecation warnings to use language like "in a future release of lightgbm" instead of "in the 4.0.0 release". This way if something happens and we end up not making all of these changes in exactly 4.0.0 (i.e. in case we make mistakes), that warning would still be accurate.

Since LightGBM doesn't do long-term support release (i.e. there wouldn't be a 3.3.1 release after 4.0.0 is out), I don't think it's necessary to list a specific future version in deprecation warnings.

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

But I have a mild preference for these deprecation warnings to use language like "in a future release of lightgbm" instead of "in the 4.0.0 release".

I think it's better to be explicit and specify exact version we're targeting. I understand your worries, but these changes are quite easy to implement and I hope nothing can prevent us to include them in 4.0.0. As opposite, we cannot be 100% sure that there will be no 3.3.1 release before 4.0.0. As you've said, we can end up having serious bug in 3.3.0 that will require releasing hotfix version 3.3.1. In that case "in a future release of lightgbm" will point to 3.3.1 version. It seems to me misleading. WDYT?

@StrikerRUS StrikerRUS changed the title [WIP][python]deprecate silent and standalone verbose args. Prefer global verbose param [RFC][python]deprecate silent and standalone verbose args. Prefer global verbose param Aug 31, 2021
@StrikerRUS StrikerRUS marked this pull request as ready for review August 31, 2021 19:17
@jameslamb
Copy link
Collaborator

In that case "in a future release of lightgbm" will point to 3.3.1 version. It seems to me misleading. WDYT?

It wouldn't be misleading in that example, because in that situation I expect that these deprecations warnings would still be left in the library in a 3.3.1 release. So both 3.3.0 code and 3.3.1 code would say "in a future release of lightgbm" if you are using the deprecated behavior.

I think that "a future release" is general enough that it won't / shouldn't be interpreted to mean "the next release".

As a general rule, I just think that log messages in libraries shouldn't be used as a source of information about very specific future versions, since we can't say with certainty right now whether the release that these deprecations will be removed in will be 4.0.0 or 4.0.1 or 4.1.0 or 5.0.0. We have a specific plan and it seems that the most likely situation is that these deprecations will become errors in exactly 4.0.0, but I don't think that's guaranteed to be true.

@StrikerRUS
Copy link
Collaborator Author

I think that "a future release" is general enough that it won't / shouldn't be interpreted to mean "the next release".

Ah, here is where we are thinking differently. For me personally "a future release" === "the next release".

As a general rule, I just think that log messages in libraries shouldn't be used as a source of information about very specific future versions, ...

This statement is quite controversial, I think. For instance, scikit-learn has very strict and formal rules of a deprecation process and always specify target version for a removal.

As in these examples, the warning message should always give both the version in which the deprecation happened and the version in which the old behavior will be removed. If the deprecation happened in version 0.x-dev, the message should say deprecation occurred in version 0.x and the removal will be in 0.(x+2), so that users will have enough time to adapt their code to the new behaviour.
https://scikit-learn.org/stable/developers/contributing.html#deprecation

Example: https://github.com/scikit-learn/scikit-learn/blob/786c4e5e39648127f41913d4e0c53672a44306f0/sklearn/tree/_classes.py#L338-L346

So, I believe it depends on development policy whether warnings should be explicit or not.

However, I don't have a strong preference and will rephrase all warnings.

@jameslamb
Copy link
Collaborator

However, I don't have a strong preference and will rephrase all warnings.

Totally hear your concerns, and I think the scikit-learn example is an excellent counterpoint. Thanks very much for making this change.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

looks great, thanks for doing this before 4.0.0!

@jameslamb jameslamb changed the title [RFC][python]deprecate silent and standalone verbose args. Prefer global verbose param [RFC][python] deprecate silent and standalone verbose args. Prefer global verbose param Sep 1, 2021
@jameslamb jameslamb merged commit 64f1500 into master Sep 4, 2021
@jameslamb jameslamb deleted the deprecate_silent branch September 4, 2021 17:03
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants