DM-27131: Reorganize pickling to allow subclasses to add parameters #153
Conversation
Use a factory unpickler in conjunction with a new method that forms the keyword arguments to use in the constructor. This allows subclasses to add additional parameters without requiring they all implement a compatible reduce method.
| def __reduce__(self): | ||
| """Pickler. | ||
| """ | ||
| return self.__class__, (self.config, self._name, self._parentTask, None) | ||
| return self._unpickle_via_factory, (self.__class__, [], self._reduce_kwargs()) |
There was a problem hiding this comment.
I think you might want to look at __getnewargs_ex__ instead of __reduce__.
There was a problem hiding this comment.
I continually try to look at it and then people always tell me they prefer reduce instead. Also, wouldn't that require that a __new__ is implemented?
There was a problem hiding this comment.
I've been switching the classes where I had used __getnewargs_ex__ to use __reduce__ with this kind of trampoline, because I don't think it's worth it to implement __new__ and explicitly disable __getstate__ and __setstate__ just to be able to use __getnewargs_ex__. I'm not sure __getstate__ and __setstate__ are always in play, but I really don't like having to think about when they might be, and how many other invisible construction paths I have to guard against. __reduce__ (with or without a trampoline) basically guarantees __init__ is called, and I think that simplicity is really underrated in the pickle docs' recommendations to prefer other approaches.
There was a problem hiding this comment.
@TallJimbo: I see the difference here from where I have used this in the past. You want to explicitly want to re-trigger the behavior in init rather than just save the state of something and restore that state (which is what you get with it just calling object.__new__ (or your own __new__), __init__ is never called a new object is just created with the specified state. FWIW, you dont need get and set state with with __getnewargs_ex__, its (yet) another way to do pickling.
There was a problem hiding this comment.
Yes, in this case the __init__ is doing a lot of mangling of the input arguments and I really don't want to be trying to keep track of all the internal attributes that are set.
| kwargs : `dict` | ||
| Keyword arguments to be used when pickling. | ||
| """ | ||
| return dict(config=self.config, name=self._name, parentTask=self._parentTask,) |
There was a problem hiding this comment.
Doesn't PipelineTask need a _reduce_kwargs() override that includes initInputs?
Use a factory unpickler in conjunction with a new method that
forms the keyword arguments to use in the constructor. This
allows subclasses to add additional parameters without requiring
they all implement a compatible reduce method.