Cached task raising tenacity exception crashes about prefect HOT 5 OPEN

I'm not sure this is something we'd want to add tenacity specific handling for.

I imagine the common usage pattern would be to define a serializer that works for your expected / successful return value (since that's what you want to cache). The fact that your serializer needs to handle all possible unhandled exceptions, even though you have no intent to cache them, is very surprising behavior. What if some other situation arises where an exception is raised that isn't json serializable?

I agree that tenacity specific handling probably isn't warranted, but I wonder if it's possible to implement more generic handling that allows retries to be executed, and task state to be correctly assigned (failed) even when the exception isn't serializable?

Does using an alternate serializer solve this issue for you?

I've implemented some other handling here - now that I know of this particular issue, I can catch this specific exception and replace it with something that's serializable by default. But I also think it's something Prefect should probably be able to handle by default.

from prefect.

Thanks for raising this one @thundercat1 and for providing workarounds in case others come across it. I've added this issue to our backlog.

from prefect.

hi @thundercat1 - I think this issue has mostly to do with the fact that you cannot pickle a _thread.RLock object in general

import pickle
import threading

def minimal_repro():
    lock = threading.RLock()
    try:
        pickle.dumps(lock)
    except TypeError as e:
        print(f"Pickling error: {e}")

if __name__ == "__main__":
    minimal_repro() # Pickling error: cannot pickle '_thread.RLock' object

if you use result_serializer="json", your example works (i.e. fails as expected). I'm not sure this is something we'd want to add tenacity specific handling for. Does using an alternate serializer solve this issue for you?

from prefect.

The core problem here appears to be that we are persisting (and therefore serializing) exceptions; in general I don't like this and don't think it's necessary - I'll look at whether any functionality depends on that and whether we can safely remove it. That being said, I want to callout that if a user uses a distributed system (like Dask or Ray), then the same error would occur because those systems would need to serialize the exception to send it across the network. I think we can look at opening an issue in tenacity to avoid maintaining a lock reference on raised exceptions.

from prefect.

Ha, looks like someone has raised this issue with tenacity already: jd/tenacity#429

from prefect.