FEA Add EngineAwareMixin to factor the common logic of estimators which are plugin-extendable

[betatim]

Adds a mixin for estimators to use when they support having an engine.

Also adds a decorator that can be used on fit to convert the estimators attributes to either engine_native or sklearn_native types.

WDYT?

Notes: I considered using something like __init_subclass__ to automagically wrap fit but decided that "explicit is better than implicit", so this is how we got the decorator approach. After building something based on __init_subclass__, which seemed cool because it means all you have to do is add the mixin, it seemed a bit too magical that this would wrap fit. So I switched to having a decorator.

[fcharras]

Sorry I wasn't aware of this PR until now. Will take a look !

[ogrisel]

This looks great! Just a few minor suggestions below:

[betatim]

Implemented the suggestions almost exactly as suggested. Made a change to the docstring suggestion and renamed the conversion function (not sure I like the name but it seems better than "convert to numpy", when really it is "to sklearn types").

[betatim]

@ogrisel what do you think about the changes in the last commit? This makes it possible to perform input validation in the "acceptance" function. Ideas on making the return value better (I dislike the True, input_vals_dict thing. Maybe the engine should store the values on its instance (and we don't pass them in to prepare_fit and so on?)? That would make it clear that the engine can do what it wants with the values (modify them, convert them, etc) and that they won't change between calling validate and the other steps of fitting.

[ogrisel]

Thanks for the update @betatim. Here is a first pass of feedback.

I would also love to hear from @fcharras on the API design proposed by team and my own suggestions.

[ogrisel]

I have the feeling that we should factor in the calls to self.estimator._validate_data and remove the _check_test_data method and simplify the _prepare_fit method.

Otherwise, the default engine would not behave in a consistent way with non-default engine and therefore would not serve as an educational implementation template for third party engine implementers.

[jjerphan]

Thanks, @betatim.

At this stage, this LGTM. I agree with @ogrisel's remarks and have a few comments.

[jjerphan]

LGTM for now. I think this is a no-op generally.

[jjerphan]

Note for future iteration: Could method decorated by convert_attributes be called concurrently? If it is the case, should we add some handling to make converting attributes safe?

[ogrisel]

I think we can assume that calling fit concurrently on the same estimator instance is not thread safe.

However, calling fit on different clones of the same estimator should always be thread-safe.

[fcharras]

Thanks for the work @betatim

I also caught up with the PR, I like the overall approach, don't have anything to add beside what's has already been suggested.

In particular like @ogrisel suggested I think _get_engine is supposed to return input validated by the engine even when hasattr(self, "_engine_provider") and not reset: ? or is there another reason behind not validating ? (it seems like an error since later on the prediction methods expect validated output already)

When it's merged I'll open a PR on our gpu plugin to see how it goes !

[ogrisel]

Just to make sure that this is visible, I replied in the previous thread:

#13 (comment)

[betatim]

I've updated the code so that accept does not return the inputs. We discussed this two days ago and settled on having a separate "acceptance" and "conversion/validation" phase.

To test it I also added a first draft of being able to pass an engine class to config_context(engine_provider=[SomeEngineAsAClass, "some_other_plugin_name"]). This way you can define an engine at runtime and pass it in. This solves the problem that you can't define a new engine at runtime, because you can't register a new entry point.

[ogrisel]

Thanks, this looks good to me. I would just like to better define the case when reset=False with a previously resolved engine class as explained below.

[fcharras]

So we've completed a circle 😁

It would help if you can highlight how it's intended to be used with a few examples regarding the inputs that could reasonably be inputed by a user and limit use-cases in the range of supported inputs (list of lists, input dtype that might require casting depending on the engine,...). What would you recommend to plugin developers regarding the behavior of the engine in each case (~does accept return True or False, does prepare_* raise an error and which kind of error).

If it's back to the same accept as a plugin developer I'm still tempted to come back to soda-inria/sklearn-numba-dpex#74 (where accept uses asarray and store the converted array before returning True) but improving it so that it raises errors for cases where we can be sure other engines will all fail too (e.g ndim).

If you consider it's a bad practice, I'd suggest to throw the engine instance after accept and re-instantiate later. But then, I think the acceptance criterion should be expected to be light and maybe non exhaustive, because I'd see triggering a conversion just for acceptation a bit wasteful.

[ogrisel]

If it's back to the same accept as a plugin developer I'm still tempted to come back to soda-inria/sklearn-numba-dpex#74 (where accept uses asarray and store the converted array before returning True) but improving it so that it raises errors for cases where we can be sure other engines will all fail too (e.g ndim).

I would rather not do store the attribute. I think in the case of sklearn_numba_dpex, it should only return False if the algorithm param is not Lloyd or if scipy.sparse.issparse(X) and return True otherwise. Then then the validation itself would happen in the prepare_ methods as usual and raise exceptions in case of problems.

Later we might refine the accept logic of sklearn_numba_dpex engines so that, for inputs which have a __dlpack__ method we inspect the device and it it's not a device managed by the sycl runtime, they would return False.

[fcharras]

I think in the case of sklearn_numba_dpex, it should only return False if the algorithm param is not Lloyd or if scipy.sparse.issparse(X) and return True otherwise. Then then the validation itself would happen in the prepare_ methods as usual and raise exceptions in case of problems.

👍 I'm fine with that. 👍

[jjerphan]

Small interjection: Do you think the following would be a better title for this PR ?

FEA Add `EngineAwareMixin` to factor the common logic of estimators which are plugin-extendable

[betatim]

I would rather not do store the attribute. I think in the case of sklearn_numba_dpex, it should only return False if the algorithm param is not Lloyd or if scipy.sparse.issparse(X) and return True otherwise. Then then the validation itself would happen in the prepare_ methods as usual and raise exceptions in case of problems.

Quoting this because I like it as answer to the question of how accepts should work with prepare_*.

In my mind accept should return False if either the hyper-parameters are not supported or if you know you can't convert the input data to a type that the engine supports (e.g. sparse data) or because the engine doesn't want to handle it (hypothetical example: because the input is on the wrong device).

To use an analogy: accepts is like checking if the plug on the end of a cable fits. A 220V power plug won't fit into an ethernet port -> return False. But of course, just because you have a cat5 cable with the right plug doesn't mean that the other end of the cable is also plugged into a ethernet port and that the data that will arrive is ethernet (some crazy person might actually try to send 220V via cat5 cable...).

This means input validation, like shape and such, should happen in the prepare_* methods.

I think it is good that, say, a "cupy plugin" accepts input that is a cupy array, but then raises an exception during prepare_* if something is wrong with the contents of the array. The alternative of not accepts'ing would mean that some other plugin (likely the default engine) gets the input and then raises an exception related to data conversion. Where the real problem is the contents. Getting the more specific error from the "cupy plugin" is better.

[betatim]

Small interjection: Do you think the following would be a better title for this PR ?

FEA Add `EngineAwareMixin` to factor the common logic of estimators which are plugin-extendable

That is indeed a better title. Switched to it. However, I think this PR is becoming a collection of several ideas (e.g. it "accidentally" also implements the idea of having ad-hoc engines that aren't registered via entrypoints), so IMHO we should try and wrap this one up and start new PRs for new ideas, instead of putting them all here.

[ogrisel]

LGTM with the previous and following suggestion for class filtering by engine_name.