[SWIG] Add streaming data support + cpp tests

[AlbertoEAF]

Implements #3995. See that issue for the full explanation.

Description

Adds support to train LightGBM models through SWIG from an input dataset stream in a single-pass without having to know the dataset size.

This can later be exposed to remaining language bindings as ChunkedArray is a regular C++ class.

After ingesting the data stream to the ChunkedArray, LGBM_DatasetCreateFromMats can be used as usual to create the train dataset from an array of arrays.

The ChunkedArray provides both a high and low-level API to insert elements. The latter supports loading data in parallel but must be managed manually. The former is not thread-safe and is meant for single input stream ingestion. It is very basic to use. Call add(value) repeatedly and finally get the data through data() or even data_as_void(), the latter being mandatory in SWIG.

Changes

• Add ChunkedArray
• Add tests for ChunkedArray (cannot use gtest yet, so they are still called manually)
• Add ChunkedArray_API_extensions.i
• Add SWIG class wrappers to wrap ChunkedArray as a proper Java class
• Add C++ tests to ChunkedArray using googletest

Refactor changes

• Enable cpplint for the swig/ folder and fix warnings with StringArray.

[AlbertoEAF]

I'd like to add googletest tests but the MR that already has gtest support is not merged yet (#3555). The test file must be called manually for now.

[StrikerRUS]

also ping @imatiach-msft
We will really appreciate your comments here!

[imatiach-msft]

added some comments

[AlbertoEAF]

@StrikerRUS @guolinke can we merge this?

The failing R tests are not caused by these changes as they only occur in Java's swig interface.

[jameslamb]

@StrikerRUS @guolinke can we merge this?

The failing R tests are not caused by these changes as they only occur in Java's swig interface.

Please update to the latest master. We've fixed several issues in CI over the last few days (#4010, #4042, #4032).

[StrikerRUS]

@StrikerRUS @guolinke can we merge this?

What about target release? Should this PR be "the last passenger" in terms of new features for 3.2.0 or we should wait a little bit and include it in the 4.0.0? As for me, I don't have any strong opinion and since this PR doesn't touch any existent code but add only new one, I don't mind to see it in 3.2.0.

[AlbertoEAF]

I agree with StrikerRUS. As this adds value and does not touch any existing code, it would be nice to have this in 3.2.0 so it could start being used in MMLSpark sooner rather than later, and so Feedzai's Java provider could get rid of the LightGBM fork which provides several yet unreleased features (model locale fix & streaming).

As for 4.0.0 we could improve the sparse streaming support by adding a method LGBM_DatasetCreateFromCSRs, that just like LGBM_DatasetCreateFromMats allows collecting creating a Dataset from a sparse input without coallescing first. This would further reduce peak memory usage (see discussion #3995 (comment) on memory spikes). As @StrikerRUS suggested we could also consider exposing ChunkedArray in the C interface if other people find it useful in other languages.

[AlbertoEAF]

gentle request for merge ping @guolinke :)

[StrikerRUS]

@AlbertoEAF Sorry for the delay!
Now that #3555 has been merged, could you please move tests on the appropriate place and make them GTest-compatible? Thank you!

[AlbertoEAF]

Ok @StrikerRUS , ported all the tests to googletest :)

I actually found a bug in coalesce_to that occured only in advanced usage with the low-level insertion API with explicit manual calls to new_chunk()+setitem() instead of add()'s so it was already worth it.

Hopefully this will make it easier for other devs to port C++ tests as they now have some examples, including extending gtest assertions.

[StrikerRUS]

@AlbertoEAF Great job! Thanks a lot!

Seems that SWIG wrapper should be updated with the recently added bool argument to coalesce_to() because right now SWIG wrapper compilation fails

Scanning dependencies of target _lightgbm_swig
[ 95%] Building CXX object CMakeFiles/_lightgbm_swig.dir/java/lightgbmlibJAVA_wrap.cxx.o
/__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx: In function ‘void Java_com_microsoft_ml_lightgbm_lightgbmlibJNI_int64ChunkedArray_1coalesce_1to_1_1SWIG_10(JNIEnv*, jclass, jlong, jobject, jlong, jboolean)’:
/__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx:4701:65: error: invalid conversion from ‘long long int*’ to ‘long int*’ [-fpermissive]
   ((ChunkedArray< int64_t > const *)arg1)->coalesce_to(arg2,arg3);
                                                                 ^
In file included from /__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx:939:0:
/__w/1/s/include/../include/LightGBM/utils/ChunkedArray.hpp:147:10: error:   initializing argument 1 of ‘void ChunkedArray<T>::coalesce_to(T*, bool) const [with T = long int]’ [-fpermissive]
     void coalesce_to(T *other, bool all_valid_addresses = false) const {
          ^
/__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx: In function ‘void Java_com_microsoft_ml_lightgbm_lightgbmlibJNI_int64ChunkedArray_1coalesce_1to_1_1SWIG_11(JNIEnv*, jclass, jlong, jobject, jlong)’:
/__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx:4714:60: error: invalid conversion from ‘long long int*’ to ‘long int*’ [-fpermissive]
   ((ChunkedArray< int64_t > const *)arg1)->coalesce_to(arg2);
                                                            ^
In file included from /__w/1/s/build/java/lightgbmlibJAVA_wrap.cxx:939:0:
/__w/1/s/include/../include/LightGBM/utils/ChunkedArray.hpp:147:10: error:   initializing argument 1 of ‘void ChunkedArray<T>::coalesce_to(T*, bool) const [with T = long int]’ [-fpermissive]
     void coalesce_to(T *other, bool all_valid_addresses = false) const {
          ^
At global scope:
cc1plus: warning: unrecognized command line option "-Wno-ignored-attributes" [enabled by default]
make[2]: *** [CMakeFiles/_lightgbm_swig.dir/java/lightgbmlibJAVA_wrap.cxx.o] Error 1
make[1]: *** [CMakeFiles/_lightgbm_swig.dir/all] Error 2

Also, please consider checking my some very minor comments below.

[jameslamb]

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/666715529

Status: success ✔️.

[jameslamb]

Ok @AlbertoEAF , I think we've fixed the R tests. Could you please merge in the latest changes from master, to get the fixes from #4084 ? Sorry for the inconvenience.

[AlbertoEAF]

Thanks for the R fixes @jameslamb ;)

@guolinke can you review for the v3.2.0 release? @StrikerRUS already approved. Thanks :)

[guolinke]

@AlbertoEAF sorry for miss this. I will review it today.

[guolinke]

Thank you!

[github-actions]

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.