ref PULSEDEV-36792 openml-lightgbm: Identify and fix non-closed SWIG objects

[ghost]

Because we believe this non-closed/destroyed C++ objects were causing the segmentation faults

Mostly authored by @gandola

[codecov]

Codecov Report

Merging #123 (80ba4ca) into master (76c4b30) will increase coverage by 0.22%.
The diff coverage is 81.25%.

❗ Current head 80ba4ca differs from pull request most recent head 3b96f41. Consider uploading reports for the commit 3b96f41 to get more accurate results

@@             Coverage Diff              @@
##             master     #123      +/-   ##
============================================
+ Coverage     80.14%   80.37%   +0.22%     
- Complexity      428      430       +2     
============================================
  Files            43       43              
  Lines          1516     1513       -3     
  Branches        138      138              
============================================
+ Hits           1215     1216       +1     
+ Misses          224      222       -2     
+ Partials         77       75       -2     

Impacted Files	Coverage Δ
...tgbm/LightGBMBinaryClassificationModelTrainer.java	86.56% <80.00%> (-0.30%)	⬇️
...eedzai/openml/provider/lightgbm/SWIGResources.java	84.40% <100.00%> (ø)
...eedzai/openml/provider/lightgbm/SWIGTrainData.java	100.00% <0.00%> (+9.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72801a1...3b96f41. Read the comment docs.

[gandola]

I think we shouldn't remove this line, instead we should:

 swigTrainData.initSwigTrainLabelDataArray(); // Init and copy from chunked data.
 try {
    (...)
 } finally {
    swigTrainData.destroySwigTrainLabelDataArray();
 }

The reason is because initSwigTrainLabelDataArray() actually creates a new array and clones the chuncks, and if I understood well the code, if we call this method twice without calling destroySwigTrainLabelDataArray we will leak an array of data.

We can think of an improvement by having a closeable class for this case too.

[AlbertoEAF]

Yes this is also the same situation as above @mlobofeedzai and should not be removed for the same reasons. I'd say, put a comment in the same manner as in the other case so there are no doubts as to why it's there.

I agree with @gandola , changing this to a try/finally is more idiomatic and better behaved in case of prior exceptions 👌.

[gandola]

@AlbertoEAF is there any reason for calling destroySwigTrainFeaturesChunkedDataArray() here? it looks to be an array that lives within the entire lifecycle of a SWIGTrainData instance and it is released in the close method.

If that is the case and to simplify and be more safe I would remove this destroySwigTrainFeaturesChunkedDataArray() because it may not be necessary and can cause confusion.

[AlbertoEAF]

Yes, it's wrong to delete this line. It'll unnecessarily duplicate RAM usage on the heaviest part of the job. Maybe add an inline comment with ("// Critical to save RAM. Do not remove!") along with a comment above explaining why it's here in the first place.

Transferring train data to a LightGBM Dataset has several stages:

1. Transfer data from a Pulse (OpenML) Dataset iterator (which doesn't know how large the dataset is) instance by instance to our ChunkedArrays. A ChunkedArray works as dynamically growing array of buffers automatically managed on the C++ side.
2. Create the LGBM Dataset data structure from such ChunkedArrays (buffers). // In this stage we achieve peak RAM usage (2x copies of the dataset => ~2x memory usage)
3. Delete the buffers as soon as possible (destroySwigTrainFeaturesChunkedDataArray()). Now we only have the train data on a LightGBM Dataset. (we're back to only one copy of the train dataset 👍 )

Finally we can train the LGBM model from the LGBM Dataset, which can still use ample memory during train, but won't ever reach the 2x memory usage factor.

[AlbertoEAF]

Yes this is necessary. It's in here we pass from a void ** to a LGBM DatasetHandle (https://lightgbm.readthedocs.io/en/latest/C-API.html#c.DatasetHandle) so we can pass it around with SWIG.

[ghost]

The last commit broke the build. Address later