Flaml: fix lgbm reproducibility

[dannycg1996]

Why are these changes needed?

In some cases, FLAML LGBM results are not reproducible, using the best model provided by FLAML. I can split the problem, and the solution, into two:

1. n_estimators on automl.model.model always = 1

As described in the title, the code self._model.set_params(n_estimators=best_iteration + 1) on LGBMEstimator.fit() causes the the n_estimators value to always be 1. I'm not sure why self._model.best_iteration_ is always 0 - I think it's due to using EarlyStopping? Regardless, we have no need to overwrite a correct n_estimators value, so I've removed this code (and my unit tests now pass!)

2. When a time_budget is set, LGBMEstimator sometimes is not reproducible, if the deadline is running out when the estimator is trained

This is a very similar issue to the one we recently resolved for the CatBoost model. Basically the callback causes the training of a model to differ, when the time budget is running out - thus making the loss impossible to reproduce. I've fixed this by simply deleting the if statement responsible - I think this is a clean solution.

Like with the CatBoost fix, I've also added a unit test, which fails when the if statement above is re-added to the codebase. This will help us identify if the issue is ever reimplemented somehow. It also allows others to test this issue, should they wish.

Also, am I okay to open an issue sometime to refactor this LGBMEstimator? There's a lot of defunct code, and it could be made both more readable and possibly more efficient.

Anyway, please let me know what you think!

Related issue number

Closes #1368

Checks

• I've used pre-commit to lint the changes in this PR (note the same in integrated in our CI checks).
• I've included any doc changes needed for https://microsoft.github.io/FLAML/. See https://microsoft.github.io/FLAML/docs/Contribute#documentation to build and test documentation locally.
• I've added tests (if relevant) corresponding to the changes introduced in this PR.
• I've made sure all auto checks have passed.

[thinkall]

Thank you so much for the PR, @dannycg1996 !

[dannycg1996]

Hi @thinkall, thanks for taking the time to explain - I understand why you wish to keep the early stopping!
I've added the early stopping back in as you requested - once tests pass, please feel free to merge.

[thinkall]

Thank you sooooooooo much, @dannycg1996 ! LGTM!

[dannycg1996]

Ah, it seems I have an issue with this test which I added. That's fine - we know it isn't always possible to reproduce scores when EarlyStopping is concerned! I'll tweak the test to ensure that our reproduced loss is at least as good as the loss reported by FLAML, and then it should be re-approvable/mergeable!

[dannycg1996]

Sorry about that @thinkall, I think it should be approvable and mergeable now!