Improve infrastructure for experimental dispatching of non existing methods in cuML #6148
Conversation
|
I think the only bigger picture question for this PR is: should we just implement the missing methods instead of forwarding? It would probably benefit all cuml users, be more straightforward (measure in less dunder usage :D), but it is a lot more work. We can decide in either direction, but wanted to bring it up so that we briefly talk about it and then decide. |
There was a problem hiding this comment.
LGTM, I think that the dispatching mechanism should work well :). However, what worries me a bit is that it is not a guarantee that Scikit-Learn functions will all work fine just following attributes transfers. There seems to be a lot of edge cases, and I believe that this would require some more rigorous testing. Ideally, if we want to guarantee functionality we would have to whitelist the functions that work fine for each estimator.
python/cuml/cuml/internals/base.pyx
Outdated
| and creates one if necessary. | ||
| """ | ||
| if not hasattr(self, "_cpu_model"): | ||
| self.import_cpu_model() |
There was a problem hiding this comment.
I think that the call to the import_cpu_model function is not necessary here as we already checked the presence of the _cpu_model_class attribute earlier.
|
Maybe something to add is a test that iterates overall cuml estimators and their scikit-learn equivalent and checks all attrs exist. We could extend it to instantiate each estimator, fit it and then repeat the check. |
|
For the "iterate over all estimators" part: I wrote something to do that for #6107 and am wanting to re-use it for #4753 (iterate all estimators, then filter those that accept |
dantegd
added
improvement
|
Yeah I think we need a separate PR to build the infrastructure for discovering estimators, etc. So fine for me to not do that here. Why do we need |
|
@betatim because this could prove to be a change that could break some third party libraries in ways I might not expect or might be unexpected for users, I think it should be opt-in while more extensive testing is done at least for a version cycle. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
…id potential infinite recursion in hdbscan
|
/merge |
This PR adds methods in UniversalBase, so that cuML estimators that inherit from it can enable better errors, and experimentally dispatch to other libraries (sklearn, umap, hdbscan...) for methods that haven't been implemented in cuML itself.