[python-package] allow use of early_stopping_round<=0 to turn off early stopping (fixes #6401)

[ddelzell]

Fixes #6401

The early stopping callback was instantiated if early_stopping_round was found in the parameter list, even its values was <=0 (which should disable early stopping). The code was modified to only instantiate the early stopping callback if early_stopping_round was both an integer and >0.

Various error messages were updated and unit tests were added.

[ddelzell]

@microsoft-github-policy-service agree company="Grubhub"

[jameslamb]

Thanks! I've added the statement "fixes #6401" to the description. In case you're not familiar with GitHub... that's helpful for 2 reasons:

1. it means [python-package] Early stopping callback added when early_stopping_round = 0 #6401 will automatically be closed when we merge this PR
2. it means that anyone finding [python-package] Early stopping callback added when early_stopping_round = 0 #6401 from search will see a link back to this PR

More details on that: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

[ddelzell]

Ah, looks like I forgot to run some linting pre-commit hooks? I can try to fix those. Also, we typically rebase to one commit in our PRs as we make changes. Do you have any problem with that?

[jameslamb]

Thanks so much for doing this! I left one comment for you to consider. Will try to provide a more thorough review later.

[jameslamb]

Ah, looks like I forgot to run some linting pre-commit hooks? I can try to fix those

Yep. You can do this from the root of the repo to run them.

pip install pre-commit
pre-commit run --all-files

we typically rebase to one commit in our PRs as we make changes. Do you have any problem with that?

We squash all commits into 1 when merging here, so we end up with 1 commit per PR. Check this out: https://github.com/microsoft/LightGBM/commits/master/

Because of that:

• you could push 1 or 10 or 100 commits to this PR and it won't be a problem
• you don't need to worry about commit messages being useful... only the PR title will be used as a commit message when this is merged

So please don't rebase and force-push here. It's useful for reviewers to be able to see the individual changes being made in response to review comments, and all the CI logs are tied to commits so it's useful to be able to compare consecutive CI runs.

[jameslamb]

Thanks very much for this! The tests look great, I just left some small suggestions. This is pretty close, and we'll get it into the next release.

[jameslamb]

Thanks for the changes!

I've updated this to the latest state of the master branch and converted it from "draft" to "ready for review", to pull in some fixes to CI that were preventing a lot of the Python tests from running (#6407). I'll provide another review in a bit.

[jameslamb]

Thanks very much, we'd love to have you come back and contribute again in the future!

[ddelzell]

Thanks, James! Great process, I learned a lot and your reviews were incredibly thorough.

[ddelzell]

@jameslamb Do you know when the next release is scheduled to be?

[jameslamb]

We're hoping to do one within the next month... but we're battling severe issues in CI right now 😭 (#6425).

[ddelzell]

OH NO. Thank you for the reply, that's helpful to know.