Add as_sklearn and from_sklearn APIs to serialize to CPU sklearn-estimators for supported models
#6102
Conversation
|
Why have the methods do both the conversion cuml<>sklearn and the serialisation? Having a way to convert to and from scikit-learn seems like a useful thing by itself. Maybe because you have your own way of serialising the model, or because you need a particular type of model or who-knows-what. So to serialise it you'd do something like How hard would it be to have Name bike shedding: if we don't save things to a file, how about If we do save to a file, then |
|
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. |
dantegd
changed the title
to_sklearn and from_sklearn APIs to serialize to CPU sklearn-estimators for supported modelsas_sklearn and from_sklearn APIs to serialize to CPU sklearn-estimators for supported models
|
@betatim just implemented your suggestions and changed the functionality to not save a file but return an estimator. I think the idea of |
| return self._cpu_model | ||
|
|
||
| @classmethod | ||
| def from_sklearn(cls, model): |
There was a problem hiding this comment.
Would be great to have a global conversion table, so that we don't need to provide the class as a parameter.
There was a problem hiding this comment.
This is a class method, so we get the class from that, it's not something the user passes (like self in non class methods)
A global conversion table will be useful for a follow up to add cuml.from_sklearn library type of functionality though
| """ | ||
| estimator = cls() | ||
| estimator.import_cpu_model() | ||
| estimator._cpu_model = model |
There was a problem hiding this comment.
It could be interesting to add an optional parameter to this function to allow a deepcopy of the sklearn model.
There was a problem hiding this comment.
lol I asked the same thing before reading this suggestion :)
There was a problem hiding this comment.
Rather than adding parameters to control deep-copying, why not implement explicit (deep-/)copying logic on the class and thus enable the user to perform the copies when desired? That would follow "Explicit is better than implicit" and "There should be one-- and preferably only one --obvious way to do it."
Example:
from copy import deepcopy as dc
accel_est = dc(cpu_estimator.from_sklearn())Supporting copies explicitly in a controlled manner would be a good idea anyways, no?
There was a problem hiding this comment.
I like Simon's suggestion. I think having a parameter is not so useful, if the above code snippet works. Then those who know they need to deepcopy can do that
python/cuml/cuml/internals/base.pyx
Outdated
| self.import_cpu_model() | ||
| self.build_cpu_model() | ||
| self.gpu_to_cpu() | ||
| return self._cpu_model |
There was a problem hiding this comment.
Should we not return a deep copy here?
There was a problem hiding this comment.
For my education, why would we want to deepcopy? Mostly asking because in my experience 99% of cases where someone uses deepcopy there is something else that we can do instead or just not do it. Mostly Python "just works" without deepcopy'ing, hence my interest
There was a problem hiding this comment.
If we simply return a reference to the internal model, any modification to one (additional training or something else) would affect the other. This might create a situation in which the CPU and GPU attributes are out of sync in the cuML estimator. Or inversely, the sklearn estimator returned by the function might silently be updated by the cuML estimator.
There was a problem hiding this comment.
perhaps it should be a parameter, by default most users probably won't care about needing a deep copy, so I wouldn't do it by default, but if a user needs it then they can request it, what do you guys think?
There was a problem hiding this comment.
I'd advise against adding such parameter and instead expect users to explicitly create a deep copy when desired. See also my other comment on this.
| else: | ||
| raise ValueError(f"Serializer {format} not supported.") |
There was a problem hiding this comment.
Does click not take care of invalid values being passed :(
There was a problem hiding this comment.
It does, I added it by accident based on habits :P
| # Convert to sklearn estimator | ||
| sklearn_estimator = accelerated_estimator.as_sklearn() | ||
|
|
||
| # Save using chosen format | ||
| with open(output, "wb") as f: | ||
| serializer.dump(sklearn_estimator, f) | ||
|
|
||
| # Exit after conversion |
There was a problem hiding this comment.
Do we need the comments? They seem like they repeat what the code says. I like comments that explain why the code is the way it is, but I don't think we need that here as it is pretty straightforward
| @pytest.fixture | ||
| def random_state(): | ||
| return 42 |
There was a problem hiding this comment.
Do we really need this instead of using 42 in the tests directly?
We could have a global version of this that allows us to run the tests with several seeds, but maybe something to tackle in the future/new PR
There was a problem hiding this comment.
We don't really need it at all
Co-authored-by: Tim Head <betatim@gmail.com>
dantegd
added
feature request
|
/merge |
No description provided.