HIPS / autograd

Efficiently computes derivatives of NumPy code.
MIT License
7.03k stars 915 forks source link

return in finally swallows exceptions #651

Open iritkatriel opened 4 weeks ago

iritkatriel commented 4 weeks ago

In https://github.com/HIPS/autograd/blob/195e8d839d93e2ffe7397f4058925a63fa0f7564/autograd/wrap_util.py#L36 there is a return statement in a finally block, which would swallow any in-flight exception.

This means that if an unhandled exception (including a BaseException such as KeyboardInterrupt) is raised from the try body, it will not propagate on as expected.

If the intention is to suppress all exceptions, I would propose to make this clear by using "except BaseException".

See also https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions.

fjosw commented 3 weeks ago

Hi @iritkatriel, thanks for your suggestion. I'm not sure about the original intention behind this line but also suspect that it is supposed to suppress all exceptions. I added

except BaseException:
    pass

in #653, is this what you had in mind?

iritkatriel commented 3 weeks ago

Then you don't need the finally clause anymore. Just have the return dedented to the level of the except.

j-towns commented 3 weeks ago

I could be wrong, but I'm fairly confident that the intention was just to catch AttributeErrors from the assignments, which I guess could occur when wrapping a callable object that is not an ordinary Python function. Something like

def wraps(fun, namestr="{fun}", docstr="{doc}", **kwargs):
    def _wraps(f):
        try:
            f.__name__ = namestr.format(fun=get_name(fun), **kwargs)
            f.__doc__ = docstr.format(fun=get_name(fun), doc=get_doc(fun), **kwargs)
        except AttributeError: pass
        return f
    return _wraps

I'm not sure why the function was written in the way it was.