use cloudpickle to serialize expressions held in an estimator + skip eager pickling checks #1324
Conversation
skrub/_expressions/_expressions.py
Outdated
| @@ -420,7 +420,8 @@ def __get__(self, instance, owner=None): | |||
| return f"""Skrub expression.\nDocstring of the preview:\n{doc}""" | |||
|
|
|||
|
|
|||
| class Expr: | |||
| class Expr(_CloudPickle): | |||
| _cloudpickle_attributes = ["_skrub_impl"] | |||
There was a problem hiding this comment.
Is _skrub_impl guaranteed to not hold a reference to a large datastructure? Calling cloudpickle.dumps would be problematic otherwise (by allocating a large intermediate datastructure before serializing to disk/network).
There was a problem hiding this comment.
right, that's true. no it isn't and in fact it is quite likely to hold references to dataframes that users passed when initializing a variable
all in all I think we might better off removing the pickle-ability checks, and any attempt to force usage of cloudpickle, and just recommend in the documentation that users rely on cloudpickle if pickle cannot handle their pipeline (eg due to lambda expressions)
There was a problem hiding this comment.
or maybe we could have that strategy on the estimators returned by get_estimator, get_grid_search etc instead because those don't have references to data, and they are more likely to be what one wants to serialize anyway after fitting
jeromedockes
changed the title
jeromedockes
added
no changelog needed
expressions
jeromedockes
changed the title
one possible fix for #1323