python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
781 stars 108 forks source link

concurrent.futures.ProcessPoolExecutor emits a very useless error message because the cattrs Exception cannot be pickled #458

Closed petergaultney closed 7 months ago

petergaultney commented 7 months ago

Description

I am structuring an attrs class with a dictionary. It fails with the following traceback:

|   File "/usr/local/lib/python3.8/site-packages/cattrs/converters.py", line 334, in structure
|     return self._structure_func.dispatch(cl)(obj, cl)
|   File "<cattrs generated structure thds.directory_synth.dm_elements.Type1_entity>", line 87, in structure_Type1_entity
|     if errors: raise __c_cve('While structuring ' + 'Type1_entity', errors, __cl)
| cattrs.errors.ClassValidationError: While structuring Type1_entity (1 sub-exception)
+-+---------------- 1 ----------------
  | Traceback (most recent call last):
  |   File "<cattrs generated structure thds.directory_synth.dm_elements.Type1_entity>", line 35, in structure_Type1_entity
  |     res['generational_suffix'] = __c_structure_generational_suffix(o['generational_suffix'], __c_type_generational_suffix)
  |   File "/usr/local/lib/python3.8/site-packages/cattrs/converters.py", line 646, in _structure_optional
... etc

Actually, the cattrs error itself is fine and expected!

What's not expected is that part of the exception that is getting raised seems not to be picklable. This leads to a horrible error message (which is mostly concurrent.futures's fault - why they won't tell you outright what they're failing to pickle is beyond me) when this fails in a worker process:

 concurrent.futures.process._RemoteTraceback:
 '''
 Traceback (most recent call last):
   File "/usr/local/lib/python3.8/concurrent/futures/process.py", line 368, in _queue_management_worker
     result_item = result_reader.recv()
   File "/usr/local/lib/python3.8/multiprocessing/connection.py", line 251, in recv
     return _ForkingPickler.loads(buf.getbuffer())
 TypeError: __new__() missing 2 required positional arguments: 'name' and 'type'
 '''

That's it! I get no actual information about what failed - just a completely useless, not-googleable error. Again, this is terrible on concurrent.futures's part - they need to at least tell me which class they're trying to call __new__ on.

After digging, while I can't be 100% certain, it seems very likely that the offending class is AttributeValidationNote.

If it wouldn't be too much to ask - a lot of Python exceptions are picklable, and it would be nice if the ones raised by cattrs were too, so that cattrs can get used without giving me headaches in a process concurrency situation. If there's some reason we have to use __new__ here (and I'm sure it wasn't chosen idly) - maybe we could implement __getstate__ for the containing exception class so that is just omits the AttributeValidationNote whenever the object is pickled? I'd rather get some information than no information at all!

Tinche commented 7 months ago

Howdy,

yeah, that sounds like AttributeValidationNote. It's a subclass of string (I think it has to be to play nice with PEP 678 with with an overridden __new__).

I'd be ok with (reasonable) changes to make AttributeValidationNote picklable. Can you look into it and put together a PR?

petergaultney commented 7 months ago

i will see if this approach might work, which would be ideal. https://stackoverflow.com/questions/56711718/pickle-object-with-overloaded-new

petergaultney commented 7 months ago

hahaha...

I started working on this this morning, and I couldn't reproduce the behavior on master... because you've already added __getnewargs__.

thanks for the quick turnaround :)

Tinche commented 7 months ago

I have no memory of this tbh, so it was another contributor ;)

This change is already out, if you update to 23.2.1 you should get it!

Can we close this?

petergaultney commented 7 months ago

yep, closing!

petergaultney commented 7 months ago

making a searchable note that this also occurs with things that are not concurrent.futures, in which cases you can get BrokenProcessPool exceptions.