-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Regression Tests and gitlab CI support #4228
Conversation
1. Use random access for data sampling 2. Support read data from multiple input files 3. Read data in batch so no need to hold all data in memory
validation dataset ignored
try remove git submodules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your interest in LightGBM!
But I don't understand this pull request, sorry. Is there some previous discussion that you've had with maintainers here that describes this work?
My initial observations:
- I'm -1 on supporting new CI pipelines on GitLab. This project already has extensive testing on GitHubAction, AppVeyor, and Azure DevOps and I'd like to understand why it's necessary to add GitLab.
- It's not clear to me why the new tests added in this PR need to be in their own folder called
regression/
instead of being included in LightGBM's existing testing infrastructure. I'd like to understand why that is being proposed.
@jameslamb Intention for this PR is to purpose what LightGBM seems to be lacking: Regression test that ensures LightGBM produces same outputs (intermediate results, models, predictions, etc) over commits or releases. GitLab CI here serves as a demo. We have been testing legally protected datasets (e.g. Yahoo), as well as large ones (e.g. Higgs) on self hosted server. GitLab CI integrates well to our current testing infrastructure, also does not affect current public Github CI. However considerations are: Regressions tests on a ML framework could be very different to traditional ones that requires each in-output to be strictly unchanged, restrictions could be added where needed or instead prompting user for change if not fatal. TL;DR
|
Thanks for the clarification. Even though there isn't a separate directory in this project called
We'd welcome a discussion about specific types of tests that the project is missing today, and then small, focused pull requests to add them one at a time. Could you please close this pull request and open a feature request at https://github.com/microsoft/LightGBM/issues describing the types of tests that you believe LightGBM is missing? Please be as specific as possible, for example:
This pull request in its current state needs significant changes before it is likely to be accepted by maintainers, so I think this topic would benefit from some discussion first. |
I agree with @jameslamb . There is no doubt that regression tests are very important. However, according to the provided definition,
such tests can be written in quite different formats, not necessary as a special GitLab jobs. I believe we already have regression tests for
LightGBM/tests/python_package_test/test_engine.py Line 2662 in 4530ded
internal model structure, LightGBM/tests/python_package_test/test_basic.py Lines 47 to 48 in 4530ded
printed outputs
All these asserts check that LightGBM performs the same way it did in previous commit. I guess this is what regression tests are about. I believe we should add more such tests with hardcoded expected results and make them more strict (for metrics on test datasets, for example) and pay more attention to PRs which change that values (#4349). Contributions are very welcome 😉 ! Sorry, but I think this PR in its' current state is not going to be merged. |
I've created two issues to track specific pieces of work that I think are relevant to the discussion in this pull request.
Since it has been almost two months since #4228 (comment) with no comment from non-maintainers on this pull request, I am closing this pull request and locking the conversation. For anyone arriving at this discussion who is interested in contributing such regression tests, please comment on #4406 / #4407 or open a new issue. |
This PR:
Example Result: on gitlab
Caveats/Future Plans: