You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I've been (slowly) working through our list of estimators applying a set of common cleanups. Rather than open an issue for each estimator I figured I'd open a tracker issue instead. I view this work as half paying down tech-debt and half fixing bugs.
Each estimator has their own warts, but in general we're interested in applying the following actions to each estimator:
Simplifying __init__. Estimators should have a simple __init__. No hyperparameters should be validated or mutated in __init__, any validation/modification needed should happen at the time of use in e.g. fit. See the sklearn docs for more here. An __init__ should just set attributes for all hyperparameters and return, nothing more. This also includes removing setting fitted attributes to None in __init__. There's no reason to do this.
Removing mutation of hyperparameters elsewhere in the model. If a value should be inferred/thresholded/amended in some way, that should happen locally in e.g. fit and leave the original value unmodified. It's important that Model(**params).get_params() == Model(**params).fit(...).get_params().
Removing unnecessary extra state. Many of our estimators stash a bunch of unnecessary state on the estimator in either public (common ones are dtype, n_cols, ...) or private attributes. Some of these are never used again and can be trivially removed. Others are very very cheap to compute from an existing fitted attribute (e.g. self.dtype always is the same as self.some_array_.dtype). Unnecessary extra state makes it hard to ensure an estimator is consistent and created correctly in all the ways we create them (init-and-fit, unpickling, from_sklearn, cuml.accel, ...). The less state the better.
Fixing memory management. Many estimators allocate values on the heap with new/malloc/libcuml apis, cast the pointer to an int and store/pass that pointer around. Sometimes that pointer is never freed, other times it is but in ways that are buggy and can result in double-free/memory leaks/segfaults. In simple cases (like parameter struct instantiation) moving to stack allocation is the solution. In other cases we define a small cdef utility class to manage the resource properly from python, then rely on python's reference counting and GC to handle lifetimes.
Checking we're applying standard type reflection properly.
Checking that the estimators are pickled properly. Most don't need to do anything extra here, this is mainly relevant when complicated memory management is needed.
Releasing the GIL for libcuml calls. It's not enough to annotate a native call signature with nogil, you also need to call it within a with nogil block.
General code hygiene and cleanups. Over the years many estimators have become a bit convoluted as new options were tacked on.
Some issues I've found and fixed while doing this have resulted in segfaults or memory leaks, others result sklearn incompatibilities affecting cuml.accel. Cleanups like this are important if a bit monotonous, for now I'm prioritizing our "gem" estimators and anything wrapped in cuml.accel already.
Below is a list of all estimators done and remaining.
I've been (slowly) working through our list of estimators applying a set of common cleanups. Rather than open an issue for each estimator I figured I'd open a tracker issue instead. I view this work as half paying down tech-debt and half fixing bugs.
Each estimator has their own warts, but in general we're interested in applying the following actions to each estimator:
__init__. Estimators should have a simple__init__. No hyperparameters should be validated or mutated in__init__, any validation/modification needed should happen at the time of use in e.g.fit. See the sklearn docs for more here. An__init__should just set attributes for all hyperparameters and return, nothing more. This also includes removing setting fitted attributes toNonein__init__. There's no reason to do this.fitand leave the original value unmodified. It's important thatModel(**params).get_params() == Model(**params).fit(...).get_params().dtype,n_cols, ...) or private attributes. Some of these are never used again and can be trivially removed. Others are very very cheap to compute from an existing fitted attribute (e.g.self.dtypealways is the same asself.some_array_.dtype). Unnecessary extra state makes it hard to ensure an estimator is consistent and created correctly in all the ways we create them (init-and-fit, unpickling,from_sklearn,cuml.accel, ...). The less state the better.new/malloc/libcumlapis, cast the pointer to anintand store/pass that pointer around. Sometimes that pointer is never freed, other times it is but in ways that are buggy and can result in double-free/memory leaks/segfaults. In simple cases (like parameter struct instantiation) moving to stack allocation is the solution. In other cases we define a smallcdefutility class to manage the resource properly from python, then rely on python's reference counting and GC to handle lifetimes.GILforlibcumlcalls. It's not enough to annotate a native call signature withnogil, you also need to call it within awith nogilblock.Some issues I've found and fixed while doing this have resulted in segfaults or memory leaks, others result sklearn incompatibilities affecting
cuml.accel. Cleanups like this are important if a bit monotonous, for now I'm prioritizing our "gem" estimators and anything wrapped incuml.accelalready.Below is a list of all estimators done and remaining.
AgglomerativeClustering#7379)HDBSCANpython wrapper #6913, A few HDBSCAN cleanups #7319)KMeanspython layer #7196)cuml.decomposition#7316)cuml.decomposition#7316)cuml.decomposition#7316)cuml.ensemble#7249)cuml.ensemble#7249)cuml.feature_extraction#8575)cuml.feature_extraction#8575)cuml.feature_extraction#8575)cuml.feature_extraction#8575)ElasticNet,Lasso, andCD#7382)ElasticNet,Lasso, andCD#7382)LogisticRegression/QN/LogisticRegressionMG#7433)Ridge#7330)SGD/MBSGDClassifier/MBSGDRegressor#7504)SGD/MBSGDClassifier/MBSGDRegressor#7504)SpectralEmbedding#7326)UMAPandsimpl_set#7456)cuml.multiclass#7508)cuml.multiclass#7508)cuml.neighborscleanups #7320)cuml.neighborscleanups #7320)KernelDensityincuml.accel#7397)cuml.neighborscleanups #7320)LabelEncoder#8039)OneHotEncoderandOrdinalEncoder#8490)OneHotEncoderandOrdinalEncoder#8490)LinearSVC/LinearSVR#7376)LinearSVC/LinearSVR#7376)