zhen0
commented
3 months ago Thanks for raising this one @thundercat1 and for providing workarounds in case others come across it. I've added this issue to our backlog.
Open thundercat1 opened 3 months ago
zhen0
commented
3 months ago Thanks for raising this one @thundercat1 and for providing workarounds in case others come across it. I've added this issue to our backlog.
zzstoatzz
commented
2 months ago 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' objectif 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?
thundercat1
commented
2 months ago 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.
cicdw
commented
1 month ago 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.
cicdw
commented
1 month ago Ha, looks like someone has raised this issue with tenacity already: https://github.com/jd/tenacity/issues/429
First check
Bug summary
I have a task that calls a library function. Inside the library,
tenacityis used for retries. But when a tenacity retry exception is raised, the task crashes without executing its own retries.Reproduction
Error
Versions (
prefect versionoutput)Additional context
This only is a problem if a
cache_key_fnis included in the task decorator.Easy workarounds exist -
reraise=Truein the tenacity retryI'll be implementing one of these workarounds, but it would be nice if the core library included handling for tenacity exceptions by default.