sacred icon indicating copy to clipboard operation
sacred copied to clipboard

Allow classmethods as config scopes and captured functions

Open thequilo opened this issue 7 years ago • 5 comments

Here's a small example of what I'd like to be able to do. The real usecase is far more complex:

from sacred import Experiment

ex = Experiment()

class A:
    @ex.config
    @classmethod
    def config(cls):
        """It would be nice to have access to `cls` inside of this ConfigScope"""
        classname = cls.__name__

    @classmethod
    def captured(cls, classname):
        """Some function to be captured."""
        print(classname)

# Do capturing here because in my case the classmethod can't be captured with a decorator.
A.captured = ex.capture(A.captured)

@ex.automain
def main():
    A.captured()

Why this fails

ConfigScope

The ConfigScope discards the context of the decorated function or method. Because of this cls is not accessible from inside of the config scope. This can be solved by saving the func.__self__ attribute in ConfigScope.__init__ if func is an instance of types.MethodType and add this to preset as the first argument when calling the config scope.

captured function

Capturing a classmethod already works if the function is captured before transforming it into a classmethod:

class A:
    @classmethod
    @ex.capture
    def captured(cls, ...):
      pass

However, this is not possible in my case. To make it work even after converting the function into a classmethod I need to unwrap the function and wrap it again into a classmethod (because a classmethod is immutable so setting additional attributes like logger and signature is not possible):

class A:
    @classmethod
    def captured(cls, ...):
      pass

# This should work, but looks very ugly
A.captured = types.MethodType(ex.capture(A.captured.__func__), A)

This looks very ugly. Ingredient.capture could be modified to handle this unwrapping and wrapping if the passed function is an instance of types.MethodType.

I'd like to hear some thoughts about this and whether this could be included in sacred.

Btw, this should also work for instance methods.

thequilo avatar Jun 10 '18 09:06 thequilo

This is an interesting usecase that I haven't considered yet. Thanks for bringing it up. Unfortunately I don't have time to dig into it further now, and I will be on vacation for the next two weeks. But from a casual read of your description, it seems to me that this problem could be solved in such a way, that it does not at all impact the standard usecases of config scopes and captured functions. If so, I'd be more than happy to include an appropriate fix into the next release.

Qwlouse avatar Jun 12 '18 06:06 Qwlouse

I just noticed that doing it like shown above:

class A:
    @ex.capture
    @classmethod
    def config(cls):
        # ...

does not work, because the classmethod is not bound to any class object yet while the decorator is called. It can work (I tested that) when the config scope is added after the class definition is done:

class A:
    @classmethod
    def config(cls):
        # ...

ex.config(A.config)

To make it work with a decorator we need access to the initialized class object. Two possibilities for this are using a superclass (or metaclass) or a decorator. I tried using a decorator on the class and this also works (including minor changes in ConfigScope):

def class_decorator(cls):
    for k in dir(cls):
        attr = getattr(cls, k)
        if isinstance(attr, types.MethodType) and getattr(attr.__func__, '_use_as_config', False):
            setattr(cls, k, ex.config(attr))
    return cls


def method_decorator(m):
    m._use_as_config = True


@class_decorator
class A:
    @method_decorator
    @classmethod
    def config(cls):
        classname = cls.__name__

The same thing is true for captured classmethods also. Now this feature seems a lot more complicated to implement than I thought and the usecases are very limited. Nevertheless, I have some code that would greatly benefit from this feature as I anyways would capture the classmethods in a different place than defining them.

thequilo avatar Jun 13 '18 22:06 thequilo

The thing with decorating the classmethod directly is very complicated, especially when dealing with subclasses. What should be the value of classname for this example?

class A:
    @ex.config
    @classmethod
    def config(cls):
        classname = cls.__name__

class B(A):
    pass

At the point of definition of the config scope, the function is not bound to any class. After subclassing A, B inherited the config scope from A, but would set classname to 'B'. I think the a reasonable possibility would be to only allow adding the config scopes afterwords like ex.config(A.config), wihch makes clear which class should be passed as cls.

For capturing a classmethod we could instead of doing types.MethodType(ex.capture(A.captured.__func__), A) define a descriptor as sublcass of classmethod:

class CapturedMethod(classmethod):
    def __init__(self, f) -> None:
        super().__init__(ex.capture(f))

and use A.c = CapturedMethod(A.c.__func__), which would be aequivalent to

class A:
    @classmethod
    @ex.capture
    def captured(cls, ...):
        # ....

thequilo avatar Jun 16 '18 02:06 thequilo

Where are we with this?

kirk86 avatar Feb 13 '19 00:02 kirk86

@kirk86 I'm not sure how useful this is anymore. I solved my use case now differently. Do you need this feature?

thequilo avatar May 03 '19 11:05 thequilo