retry generator + tests #2
Conversation
|
@amiruci I will be updating https://github.com/CloverHealth/clover_pipeline/pull/5117 shortly. |
|
lgtm |
| .. code:: python | ||
| def retry(exceptions=Exception, tries=-1, delay=0, max_delay=None, backoff=1, jitter=0, logger=logging_logger): | ||
| def retry(exceptions=Exception, tries=-1, delay=0, max_delay=None, backoff=1, jitter=0, logger=logging_logger, generator=False): |
There was a problem hiding this comment.
Python compiles generator functions in a different fashion than regular functions, you can determine this via inspection instead of requiring the user to know whether a function is a generator or not: https://docs.python.org/3/library/inspect.html#inspect.isgeneratorfunction
There was a problem hiding this comment.
I was requiring the user to do so here to ensure no change in interface.
There was a problem hiding this comment.
how do you mean "no change in interface"?
There was a problem hiding this comment.
Say you have this method:
def foo():
for x in y:
yield x
Before this PR, you could @retry foo and expect certain behaviors. I don't want to change that behavior, instead requiring the developer to use generator=True to change behavior in that scenario.
| return __retry_internal(partial(f, *args, **kwargs), exceptions, tries, delay, max_delay, backoff, jitter, | ||
| logger) | ||
| pf = partial(f, *args, **kwargs) | ||
| if generator: |
There was a problem hiding this comment.
See my other comments on interface. I could, however, error if generator and not inspect.isgeneratorfunction(f).
gwax
left a comment
gwax
left a comment
There was a problem hiding this comment.
Broadly speaking, I worry that this is a dangerous tool if misused with particularly tricky edge cases around restarting a generator. I do not expect that users of this would necessarily know enough to avoid those pitfalls when wrapping a generator with @retry
I'll defer to you on this, particularly because you have more context around developers at Clover. |
|
Closing PR per @gwax recommendation we don't want to use this at Clover. I'm also going to deprecate this fork because using a fork of a python package is a bit cumbersome based on how we currently do package management. |
See invl#16