botocore WaiterError causes TypeError

[iainelder]

One of my scripts runs a CloudFormation update and uses a waiter to make the operation synchronous.

Botocove appears to iterate over all the accounts, but it crashes before I can print its output.

It turns out that WaiterError is not a copyable exception (see #25 for the analysis about copyable exceptions.)

I can't share the code or the complete trace of what I'm working on. Here's the trace that's relevant to botocove.

Executing function: 100%|██████████████████████████████████████████████| 289/289 [08:52<00:00,  1.84s/it]
Traceback (most recent call last):
[...]
  File "/home/isme/.cache/pypoetry/virtualenvs/fakeproject/lib/python3.8/site-packages/botocove/cove_decorator.py", line 63, in wrapper
    Exceptions=[
  File "/home/isme/.cache/pypoetry/virtualenvs/fakeproject/lib/python3.8/site-packages/botocove/cove_decorator.py", line 64, in <listcomp>
    dataclass_converter(e)
  File "/home/isme/.cache/pypoetry/virtualenvs/fakeproject/lib/python3.8/site-packages/botocove/cove_decorator.py", line 17, in dataclass_converter
    return {k: v for k, v in asdict(d).items() if v is not None}
  File "/usr/lib/python3.8/dataclasses.py", line 1073, in asdict
    return _asdict_inner(obj, dict_factory)
  File "/usr/lib/python3.8/dataclasses.py", line 1080, in _asdict_inner
    value = _asdict_inner(getattr(obj, f.name), dict_factory)
  File "/usr/lib/python3.8/dataclasses.py", line 1114, in _asdict_inner
    return copy.deepcopy(obj)
  File "/usr/lib/python3.8/copy.py", line 172, in deepcopy
    y = _reconstruct(x, memo, *rv)
  File "/usr/lib/python3.8/copy.py", line 264, in _reconstruct
    y = func(*args)
  File "/home/isme/.cache/pypoetry/virtualenvs/fakeproject/lib/python3.8/site-packages/botocore/exceptions.py", line 28, in _exception_from_packed_args
    return exception_cls(*args, **kwargs)
TypeError: __init__() missing 1 required positional argument: 'last_response'

You can reproduce it easily enough in the REPL:

In [1]: from botocore.exceptions import WaiterError

In [2]: error = WaiterError(
   ...:     name="FakeWaiter", reason="FakeReason", last_response="FakeResponse"
   ...: )

In [3]: from copy import copy

In [4]: copy(error)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [4], in <module>
----> 1 copy(error)

File /usr/lib/python3.8/copy.py:102, in copy(x)
    100 if isinstance(rv, str):
    101     return x
--> 102 return _reconstruct(x, None, *rv)

File /usr/lib/python3.8/copy.py:264, in _reconstruct(x, memo, func, args, state, listiter, dictiter, deepcopy)
    262 if deep and args:
    263     args = (deepcopy(arg, memo) for arg in args)
--> 264 y = func(*args)
    265 if deep:
    266     memo[id(x)] = y

File ~/.cache/pypoetry/virtualenvs/bp-1082-udpate-apt-002-product-kAdUhv53-py3.8/lib/python3.8/site-packages/botocore/exceptions.py:28, in _exception_from_packed_args(exception_cls, args, kwargs)
     26 if kwargs is None:
     27     kwargs = {}
---> 28 return exception_cls(*args, **kwargs)

TypeError: __init__() missing 1 required positional argument: 'last_response'

I'm going to raise an issue in the botocore repo to ask that the WaiterError be made copyable like the other client errors.

Why does botocore use the asdict method? It seems unnecessary to recursively copy and covert dataclasses It requires that all output and errors be copyable (and as #25 shows, for exceptions it is non-obvious to implement!) and (although this is another issue it has surprised me a couple of times when the dataclass object I retied was converted to a dict!

Would you consider changing this part to remove the use of the asdict method?

botocove/botocove/cove_decorator.py

Lines 15 to 17 in b36164b

	def dataclass_converter(d: CoveSessionInformation) -> Dict[str, Any]:
	"""Unpack dataclass into dict and remove None values"""
	return {k: v for k, v in asdict(d).items() if v is not None}

It requires that all the user's output and errors be copyable (and as #25 shows, for exceptions it is non-obvious to implement!).

[connelldave]

It's a shame this shim is causing pain: when I wrote it it was definitely just a deferred decision until I figured out a better way. The intent was a noop that I could make a harder decision on later. Lesson learned - apologies!

The motivation was to give Botocove an explicit output object type that was useful and worthwhile to pass around: I used a dataclass as a bit of an experiment and then shimmed it back to it's previous implementation to not break backwards compatability, and because I didn't want to commit to dataclass objects for end users yet. We do a lot less passing around in the re-write that brought in tqdm, and now the releaseable-session change.

I still don't really feel like data classes are really adding any value, so the simple solution (I think?) here is to just drop them and use a TypedDict and not mess around having to convert them as a consequence.

I'm still mentally deferring a decision on better output data shape for a future 2.0 release here, I haven't really had any inspiration for what's worthwhile - maybe it's fine as is?

[iainelder]

I still don't really feel like data classes are really adding any value, so the simple solution (I think?) here is to just drop them and use a TypedDict and not mess around having to convert them as a consequence.

Yeah, if internally they don't add much value and they are being converted back to dataclasses anyway for the client then probably the right thing to do long-term is to bring back the TypedDict implemention used in 1.3.0.

I'm still mentally deferring a decision on better output data shape for a future 2.0 release here, I haven't really had any inspiration for what's worthwhile - maybe it's fine as is?

If you mean the three main keys Results, Exceptions and FailedAssumeRole, then I don't have any compelling reason to change that right now. I sometimes find it more useful to chain the iterables together and process a single stream of results. But that's easy enough to do with itertools.chain on a case-by-case basis.

The intent was a noop

Unfortunately the undesirable deep-copy behavior can't even be changed. There's an discussion on the python-ideas list about improving it with a "recursive-or-not" flag. But I don't see it being implemented soon. https://groups.google.com/g/python-ideas/c/bWl0afPZDJM

As a quick fix I would try to replace the asdict call with an expression that builds a dictionary without recursion. It would copy only the fields of the dataclass and not traverse the iterables at all.

If you prefer to go directly to replacing the dataclass with a good old TypedDict, that's fine too.