-
-
Notifications
You must be signed in to change notification settings - Fork 31.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
Unpickling Exceptions with keyword arguments in multiprocessing throws an error #120810
Comments
Welcome 👋 I suspect that this might be a very tricky case to solve. |
Actually, the reproducer can be simplified even further. Error is triggered without import pickle
class Foo(Exception):
foo: int
bar: str
def __init__(self, foo, bar):
self.foo = foo
self.bar = bar
if __name__ == '__main__':
p = pickle.dumps(Foo(foo=42, bar="a"))
f = pickle.loads(p)
print(f) Traceback:
Removing |
It's documented in the pickle docs that classes which require keyword arguments for their constructor must implement Perhaps we can improve the docs and/or the error message, however. |
Oh, but your constructor doesn't require keyword arguments. You just use keyword arguments. Hm. |
First of all, how am I supposed to discover that if I'm using multiprocessing? I forgot that it uses pickle under the hood. Second, docs talk about
My class does not have |
Possibly related: #89275 and #89275 (comment). I don't have a setup to check the current implementation now, but maybe Irit's comment is still valid:
So it might happen that keyword arguments are simply ignored (or not remembered as such?). |
Yes, the keyword arguments are simply ignored. class Foo(Exception):
def __init__(self, x, y):
self.x = x
self.y = y
a = Foo(1, y=2)
print(a.args)
> (1,) So, should we also have a |
Alternatively, I'd suggest changing the docs to handle those cases, by saying something like "if your custom class exception supports keyword-arguments in the constructor (required or not), then you must implement a custom reduction". That's one way to easily solve the issue (and say that Or your idea of having |
We still need to update the existing exceptions, like >>> a = ImportError(name="foo")
>>> a.args
()
>>> b = ImportError("foo")
>>> b.args
('foo',)
>>> |
So, we need to decide whether we should add a |
Formally speaking, "The tuple of arguments given to the exception constructor" is correct since "argument" is not used when talking about a signature (where we talk about parameters), so it should indeed refer to what was given to the constructor call (thought we could specify that args only store the positional arguments of the call). I also prefer first changing the docs before introducing something else. |
I encountered this issue along with @maksbotan, and in my opinion, updating the Exception documentation doesn't really help here. My reasoning is as follows: The error message we received from pickle indicated that some constructor lacked certain arguments but did not specify why. In our real-world scenario, the code was more complex than the simplified example we provided, so initially, I had no idea what was causing the problem. There is no situation where my first instinct would be to scrutinize the Exception documentation. For example, we suspected that the dataclass decorator (which our Exceptions also had) might be causing the issue, so we spent some time refactoring it. After running some tests, we saw that this approach didn't help. We went through several other guesses similarly. Even if this note were in the documentation, it would still take us a considerable amount of time to find it. In my opinion, this behavior is neither natural nor expected. Sure, there may be a deep technical explanation for why this happens, but it doesn't make sense to argue that this should happen. I would prefer that Exception-derived classes are not treated as exceptions (no pun intended) by the pickling mechanism, rather than simply having this noted somewhere in the documentation. |
+1 to what @tyshchuk said. I need to pickle exceptions constructed by someone else's API, and I don't see a way to do it at all. I can't ask them to modify the exception or construct it without using kwargs -- their Exception in fact requires using kwargs to construct it, which I take no issue with, kwargs are useful. Changing the documentation won't help. Unless we can force everyone who subclasses Exception to closely read this documentation, their downstream users will run into this error with no way out. If there is some way to get around this, though, that would be really helpful to know! |
@alex-bancroft this may be too late, but with the help of the code samples in this issue, I came up with the following: import functools
import pickle
def ensure_exceptions_picklable(func):
"""
Decorator to ensure that any exception raised by the decorated function is picklable.
This is essential when using a ProcessPoolExecutor to run tasks in separate processes since all
exceptions need to be serialized (pickled) to be transferred back to the caller process. If the
exception cannot be pickled or unpickled, a new exception is raised with the original traceback.
"""
@functools.wraps(func)
def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except Exception as err:
try:
pickle.loads(pickle.dumps(err))
except Exception:
raise Exception("Unpicklable exception raised. See stack trace for the original exception.") from err
raise
return wrapper |
Bug report
Bug description:
I've tried using an
Exception
subclass with some fields and a simple constructor:When I construct and return this exception from a function invoked through
ProcessPoolExecutor
'smap
method, I get unpickling error:(For simpler reproducer see #120810 (comment))
Traceback on 3.13.0b2 from official Docker image
Same error happens on 3.10, 3.12, and also on MacOS. Different fork method (
fork
,forkserver
,spawn
) all behave in the same way, both on Linux and MacOS.However, if I construct
Foo
with positional arguments, there is no error:Output:
And, most strangely, if I remove
Exception
base class (leaving justclass Foo():
) there is no error with keyword arguments.I've found only one similar issue: #103352, but that change does not fix my issue (tested on latest 3.13 beta).
Currently we workarounded this issue in our code by using positional arguments, but it was very annoying to debug this and getting to solution actually took us a couple of days.
CPython versions tested on:
3.10, 3.12, 3.13
Operating systems tested on:
Linux, macOS
The text was updated successfully, but these errors were encountered: