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

Regression Tests and gitlab CI support #4228

Closed
wants to merge 37 commits into from

Conversation

Willian-Zhang
Copy link
Contributor

@Willian-Zhang Willian-Zhang commented Apr 26, 2021

This PR depends on #4089

Regression here stands for testing method as described here . To not be confused with term regression tasks in ML realm.

This PR:

  • Add basic regression test supports
    • create report for all tests
    • many to many relationship between testing scripts and verified datas
    • multiple regression tests
    • sha1 and color report
  • Gitlab CI Support
    • to build lightgbm on the fly
    • run regression tests

Example Result: on gitlab

Caveats/Future Plans:

  • Gitlab CI Support depends on a image I created willianz/lightgbm-build-env:latest, this won't effect current upstream CI (Microsoft) whose CI were fulled on GitHub.
  • Regression Tests not yet added to GitHub CI
  • Current check implementation relies on local data generation for referencing data
    • as regression logics (caching and moving data around) should be more dependent on CI tool side instead of checking side
    • and there would be a pending decision on how Add-Remove-Change effects CI reports (are they strictly prohibited)
  • This PR depends on [python-package] Create Dataset from multiple data files  #4089
    • Considering there are some local regression tests yet under development against Sequence feature

cyfdecyf and others added 30 commits March 21, 2021 17:57
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
@ghost
Copy link

ghost commented Apr 26, 2021

CLA assistant check
All CLA requirements met.

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.

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.

@Willian-Zhang
Copy link
Contributor Author

@jameslamb
Sorry for not making this clear.

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.
This is especially useful on project of this magnitude, specially when building, testing with large datasets takes huge amount of time or resources, while CI could step up offload those heavy tasks asynchronously during development cycle.

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.
Despite running procedure could be arbitrary (C++, Python, R or any combination of those routine) and reporting mechanism is strongly sighted to CI tool adopted, It's really up to your design decision where restrictions or suggestion to put on regression changes.

TL;DR

  1. Gitlab is just a proposal, it does not affect current GitHub CI procedure.
  2. Please consider adding Regression tests.

@jameslamb
Copy link
Collaborator

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.

Thanks for the clarification.

Even though there isn't a separate directory in this project called regression_tests, many of the project's tests meet that definition...they are intended to catch breaking changes. For example, consider these tests that check for exact measures of metrics from training on a fixed dataset:

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:

  • tests that a model file written in version X can be used in version Y
  • tests that a Dataset file written by version X can be read by version Y
  • etc.

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.

@StrikerRUS
Copy link
Collaborator

I agree with @jameslamb . There is no doubt that regression tests are very important. However, according to the provided definition,

Regression testing (rarely non-regression testing) is re-running functional and non-functional tests to ensure that previously developed and tested software still performs after a change.

such tests can be written in quite different formats, not necessary as a special GitLab jobs.

I believe we already have regression tests for
metrics,


assert log_loss(y_train, y_pred) < 0.661

internal model structure,
assert bst.lower_bound() == pytest.approx(-2.9040190126976606)
assert bst.upper_bound() == pytest.approx(3.3182142872462883)

printed outputs
assert "\n".join(actual_log_wo_gpu_stuff) == expected_log

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.

@jameslamb
Copy link
Collaborator

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.

@jameslamb jameslamb closed this Jun 25, 2021
@microsoft microsoft locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants