Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug adding handler in case of decoration + class function + filtered event #1004

Closed
sdesrozis opened this issue May 2, 2020 · 0 comments · Fixed by #1048
Closed

Bug adding handler in case of decoration + class function + filtered event #1004

sdesrozis opened this issue May 2, 2020 · 0 comments · Fixed by #1048
Labels

Comments

@sdesrozis
Copy link
Contributor

sdesrozis commented May 2, 2020

🐛 Bug description

I would like to report a bug using handler defined by decorated function in a class with filtered event.

The following code reproduces all possible situations to add an handler defined w/wo decoration in a class or not, w/wo engine (or args), using an event w/wo filter

engine = Engine(lambda e, b: b)

# decorator
def decorated(fun):
    @functools.wraps(fun)
    def wrapper(*args, **kwargs):
        return fun(*args, **kwargs)
    return wrapper

# handler as a function
def foo():
    print("foo")

# handler as a decorated function
@decorated
def decorated_foo():
    print("decorated_foo")

# register handler as a function -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo)
# register handler as a function with filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo)
# register handler as a decorated function -- OK
engine.add_event_handler(Events.EPOCH_STARTED, decorated_foo)
# register handler as a decorated function with filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), decorated_foo)


# handler as a function with engine (here args)
def foo_args(args):
    print("foo_args", args)


# handler as a decorated function with engine 
@decorated
def decorated_foo_args(args):
    print("decorated_foo_args", args)

# register handler as a function with engine  -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo_args)
# register handler as a function with engine and filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo_args)
# register handler as a decorated function with engine -- OK
engine.add_event_handler(Events.EPOCH_STARTED, decorated_foo_args)
# register handler as a decorated function with engine and filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), decorated_foo_args)

class Foo:
    # handler as a class function (ie method)
    def foo(self):
        print("foo")

    # handler as a decorated method
    @decorated
    def decorated_foo(self):
        print("decorated_foo")

    # handler as a method with engine
    def foo_args(self, args):
        print("foo_args", args)

    # handler as a decorated method with engine
    @decorated
    def decorated_foo_args(self, args):
        print("decorated_foo_args", args)


foo = Foo()

# register handler as a method -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo.foo)
# register handler as a method with filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo.foo)
# register handler as a decorated method -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo.decorated_foo)
# register handler as a decorated method with filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo.decorated_foo)
# register handler as a method with engine  -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo.foo_args)
# register handler as a method with engine and filter -- OK
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo.foo_args)
# register handler as a decorated method with engine -- OK
engine.add_event_handler(Events.EPOCH_STARTED, foo.decorated_foo_args)

# register handler as a decorated method with engine and filter -- FAILED
engine.add_event_handler(Events.EPOCH_STARTED(every=2), foo.decorated_foo_args)

engine.run([0])

The error is

Error adding <function Foo.decorated_foo_args at 0x1229b6af0> 'handler': takes parameters ['self', 'args'] but will be called with [](missing a required argument: 'self').

Why ?

First, a handler defined with a filtered event is wrapped with decoration. See https://github.com/sdesrozis/ignite/blob/93be57aa3f71ce601391d59096c3b430c4d9487b/ignite/engine/engine.py#L198. Note that functools.wraps is used to fit the signature of the related handler.

The failed case is decorated method with engine. So, I guess functools.wraps works perfectly and catch self and engine as arguments. But the signature checking search (using inspect.signature) fails because missing self...

See signature checking

def _check_signature(fn: Callable, fn_description: str, *args, **kwargs) -> None:

I think this is related to follow_wrapped=True argument of inspect.signature.

Environment

  • PyTorch Version (e.g., 1.4): 1.5
  • Ignite Version (e.g., 0.3.0): 0.4
  • OS (e.g., Linux): MacOS
  • How you installed Ignite (conda, pip, source): Honda
  • Python version: 3.7
  • Any other relevant information:
@sdesrozis sdesrozis added the bug label May 2, 2020
This was referenced May 16, 2020
vfdev-5 added a commit that referenced this issue May 16, 2020
* check signature of filtered handler rather than decorator

* add test

* autopep8 fix

Co-authored-by: AutoPEP8 <>
Co-authored-by: vfdev <vfdev.5@gmail.com>
sdesrozis pushed a commit to sdesrozis/ignite that referenced this issue May 18, 2020
vfdev-5 added a commit that referenced this issue May 18, 2020
* first attempt to fix issue_874

* autopep8 fix

* add usage

* fix bug

* autopep8 fix

* add missing cls in module

* fix bug : completed must be registered last

* autopep8 fix

* add test

* autopep8 fix

* attempt to move usage from __init__ to attach()

* autopep8 fix

* refactor metrics : split attachment from metric

* autopep8 fix

* remove EngineMetric

* add str in api

* add doc

* autopep8 fix

* improve doc

* improve doc

* fix filtered batch usage

* fix _check_signature for wrappers

* disable _assert_non_filtered_event

* add test for batch filtered usage

* revert follow_wrapped

* fix bug due to decoration and wrappers

* fix tests

* autopep8 fix

* Update test_metric.py

* remove _assert_non_filtered_event

* autopep8 fix

* update doc

* autopep8 fix

* #1004 allows to use torch.no_grad decorator in class

* autopep8 fix

* improve doc

* Update metrics.rst

* Fixed flake8 and improved docs

Co-authored-by: AutoPEP8 <>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: Desroziers <sylvain.desroziers@ifpen.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant