-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: pickle exceptions with custom __reduce__
#2747
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
base: main
Are you sure you want to change the base?
Conversation
__reduce__
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @classmethod | ||
| def _reconstruct(cls, message: str, request: httpx.Request) -> Self: | ||
| return cls(message=message, request=request) | ||
|
|
||
| @override | ||
| def __reduce__(self) -> tuple[Callable[..., Self], tuple[Any, ...]]: | ||
| return (self.__class__._reconstruct, (self.message, self.request)) |
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.
Pickling APITimeoutError passes unsupported message arg
The new APIConnectionError.__reduce__ returns _reconstruct(self.message, self.request), which reconstructs the object via cls(message=message, request=request). APITimeoutError subclasses APIConnectionError but its __init__ takes only a request parameter and hard‑codes the message. Attempting to pickle an APITimeoutError will therefore raise TypeError: APITimeoutError.__init__() got an unexpected keyword argument 'message', defeating the goal of making these exceptions picklable. APITimeoutError needs its own __reduce__/reconstructor or _reconstruct must handle subclasses without a message parameter.
Useful? React with 👍 / 👎.
Showmick119
left a comment
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.
The APIConnectionError class passes an unsupported message arg, which will raise a TypeError. Kindly address this, before next steps.
Showmick119
left a comment
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.
Looks good to me.
Changes being requested
Closes #2629. When working in a distributed environment, its useful to pickle and unpickle exceptions between workers. The default
__reduce__in Python does not account for keyword-only args, which is why we need to handle it ourselves.Alternatively, we could modify the constructor of the exception classes to not use keyword-only args, but that was a larger change.