[REVIEW] Allow saving Dask RandomForest models immediately after training (fixes #3331)

[jameslamb]

This attempts to fix #3331. See that issue for a lot more details.

Today, .get_combined_model() for the Dask RandomForest model objects returns None if it's called immediately after training. That pattern is recommended in "Distributed Model Pickling". Without this support, there is not a way to save a Dask RandomForest model using only public methods / attributes on those classes.

Per #3331 (comment), this PR proposes populating the internal model object whenever get_combined_model() is called.

Notes for Reviewers

• I have not tested this locally. I spent about 3 hours trying to build cuml from source following https://github.com/rapidsai/cuml/blob/main/BUILD.md, and was not successful. If there is a containerized setup for developing cuml, I'd greatly appreciate it and would be happy to try it out. I've added a unit test for this change, so I hope that will be enough to confirm that this works and that CI will catch any mistakes I've made.

Thanks for your time and consideration.

[hcho3]

@jameslamb I'm sorry to hear that you had trouble building cuML from the source. Here is a page for a Dockerized setup. Also, feel free to ping me if you'd like more help with building cuML (I do it at least several times a week).

[hcho3]

rerun tests

[codecov-io]

Codecov Report

Merging #3388 (eb67bd1) into branch-0.18 (550121b) will increase coverage by 0.12%.
The diff coverage is 84.78%.

@@               Coverage Diff               @@
##           branch-0.18    #3388      +/-   ##
===============================================
+ Coverage        71.48%   71.60%   +0.12%     
===============================================
  Files              207      210       +3     
  Lines            16748    16932     +184     
===============================================
+ Hits             11973    12125     +152     
- Misses            4775     4807      +32     

Impacted Files	Coverage Δ
python/cuml/decomposition/incremental_pca.py	94.70% <ø> (ø)
python/cuml/dask/ensemble/base.py	19.69% <30.43%> (+0.36%)	⬆️
...ython/cuml/dask/ensemble/randomforestclassifier.py	29.76% <40.00%> (+0.27%)	⬆️
python/cuml/dask/ensemble/randomforestregressor.py	34.42% <40.00%> (-0.12%)	⬇️
python/cuml/ensemble/randomforestregressor.pyx	70.83% <44.44%> (ø)
python/cuml/fil/fil.pyx	91.87% <60.00%> (-1.88%)	⬇️
python/cuml/ensemble/randomforestclassifier.pyx	73.72% <66.66%> (ø)
python/cuml/multiclass/multiclass.py	84.21% <84.21%> (ø)
python/cuml/model_selection/_split.py	90.35% <90.35%> (ø)
python/cuml/svm/svm_base.pyx	94.27% <91.30%> (-0.63%)	⬇️
... and 17 more

---

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 d72c54a...eb67bd1. Read the comment docs.

[JohnZed]

Looks good, and thank you for the contribution! I have two small requests.
Also, one test is failing but that is for an unrelated issue currently being fixed in #3391

[JohnZed]

Can we move these to dask.ensemble.base instead of the two versions?

[jameslamb]

Oh sure, I can try that. I didn't think that would work because cuml.dask.ensemble.base.BaseRandomForestModel inherits from object and doesn't have self._get_internal_model() defined.

But I see now that it references that method (

cuml/python/cuml/dask/ensemble/base.py

Line 172 in 816bb65

if self._get_internal_model() is None:

), so I guess that class isn't intended to be used by itself and assumes it's only used as a mixin together with cuml.dask.common.base.BaseEstimator

[jameslamb]

moved in 83f10b7

[JohnZed]

Many of the params here are either defaults or close to defaults. You could omit them to shrink the test and make it easier to maintain.

[jameslamb]

sure, no problem. I copied this exactly from the existing dask random forest tests:

cuml/python/cuml/test/dask/test_random_forest.py

Lines 366 to 378 in 816bb65

	cu_rf_mg = cuRFC_mg(max_features=1.0, max_samples=1.0,
	n_bins=16, split_algo=0, split_criterion=0,
	min_samples_leaf=2, seed=23707, n_streams=1,
	n_estimators=n_estimators, max_leaves=-1,
	max_depth=max_depth)
	y = y.astype(np.int32)
	elif estimator_type == 'regression':
	cu_rf_mg = cuRFR_mg(max_features=1.0, max_samples=1.0,
	n_bins=16, split_algo=0,
	min_samples_leaf=2, seed=23707, n_streams=1,
	n_estimators=n_estimators, max_leaves=-1,
	max_depth=max_depth)
	y = y.astype(np.float32)

[jameslamb]

updated in 83f10b7. I tried to keep a few that would have the most impact on the runtime of training, to keep the tests quick

[jameslamb]

added a fix in 6a1f2d2, sorry. I got confused by the different levels of inheritance

[JohnZed]

rerun tests

[JohnZed]

Looks good, @jameslamb ! Sorry about the delay, I wanted to also try this out locally. Pickles smoothly now.

[JohnZed]

@gpucibot merge

[jameslamb]

No problem! Thanks for the reviews, and to @hcho3 for the pointer to a dockerized setup for building cuml from source. I'll definitely make use of that in the future.