-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REF-2089] Use dill instead of cloudpickle for serialization #2922
Conversation
* smaller size pickles * support dynamically defined states * avoid issues with unpickleable globals
Instead of converting the functions up front and assigning them to the instance, unbox the function from the EventHandler when it is requested via __getattribute__. This reduces the size of the per-instance pickle, because event handler bodies do not need to be included.
Because pydantic can be installed without cython, only use the workaround in the case where the BaseModel.validate function is NOT a FunctionType, indicating it's a cython function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -2159,6 +2187,25 @@ async def modify_state(self, token: str) -> AsyncIterator[BaseState]: | |||
await self.set_state(token, state) | |||
|
|||
|
|||
# Workaround https://github.com/cloudpipe/cloudpickle/issues/408 for dynamic pydantic classes | |||
if not isinstance(State.validate.__func__, FunctionType): | |||
cython_function_or_method = type(State.validate.__func__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what type is a cython function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a cython function is compiled to C via the cython library. there's no importable name for it though, because it masquerades as a builtin, but it's not accessible as a builtin name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to the builtin function
or method
types, but those types are exposed via types.FunctionType
and types.MethodType
respectively
Dynamically convert EventHandler to functools.partial
This gets us a step closer to EventHandler as descriptor without breaking any tests or existing functionality.