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

Add default arguments to delegation #175

Open
kessido opened this issue Nov 7, 2020 · 7 comments
Open

Add default arguments to delegation #175

kessido opened this issue Nov 7, 2020 · 7 comments

Comments

@kessido
Copy link
Contributor

kessido commented Nov 7, 2020

Scenario:
You have function a, e.g:
def cnn(c_in, c_mid, c_out, act, bn=.., ..., blocks=CNNBlock): ...
and you want to add a specification for it:
def resnet(c_in, c_mid, c_out, act=relu, bn=..., ..., blocks=ResBlock): ... return cnn(...)

Right now as far as I understand this is the implementation:

use_kwargs(blocks=ResBlock)
delegates(cnn)
def resnet(c_in, c_mid, c_out, act=relu, **kwargs): 
  if 'blocks' not in kwargs: kwargs['blocks'] = ResBlock
  ...
  cnn(c_in, c_mid, c_out, act, **kwargs

So I'm thinking of doing this instead:

add_kwargs(act=relu, blocks=ResBlock)
delegates(cnn)
def resnet(c_in, c_mid, c_out, **kwargs):
  ...
  return cnn(c_in, c_mid, c_out, **kwargs)

example of implementation:

def add_kwargs(**kwargs):
    "Decorator: add argument with default value to `**kwargs` in both signature and function"
    def _f(f):
        @wraps(f)
        def _inner(*args, **kw): return f(*args, **{**kwargs, **kw})
        sig = inspect.signature(_inner)
        sigd = dict(sig.parameters)
        for k,v in kwargs.items():
            if k in sigd.keys(): 
                assert sigd[k].kind.name not in ['POSITIONAL_ONLY', 'VAR_KEYWORD', 'VAR_POSITIONAL'], \
                       f'cannot assign an existing variable ({k!r}) of type {sigd[k].kind.name}'
                sigd[k] = sigd[k].replace(default=v, kind=inspect._ParameterKind.KEYWORD_ONLY)
            else: sigd[k] = inspect.Parameter(k, inspect._ParameterKind.KEYWORD_ONLY, default=v)
        params = [[p for p in sigd.values() if p.kind == t] for t in range(5)]
        _inner.__signature__ = sig.replace(parameters=concat(*params))
        return _inner
    return _f

What do you think?

@hamelsmu
Copy link
Contributor

@kessido can you provide an example of why this would be useful? Why do you need to add additional args other than what is provided in delegates or what is in the function that is being delegated to? It would be helpful to understand the use case if you have one in mind! (it sounds like you might from the code above, but I don't want to guess).

@jph00
Copy link
Contributor

jph00 commented Dec 21, 2020

Apologies for closing in error!

@kessido
Copy link
Contributor Author

kessido commented Dec 22, 2020

Sorry quite occupied right now.
I search for the first thing I found, which might not be the best use case but I will leave it at that.

(source https://github.com/fastai/fastai/blob/7d5df9ce4b9d97b691f1ae738ed7f3ac68f89ebc/fastai/vision/gan.py#L71)

@delegates(ConvLayer)
def DenseResBlock(nf, norm_type=NormType.Batch, **kwargs):
    "Resnet block of `nf` features. `conv_kwargs` are passed to `conv_layer`."
    return SequentialEx(ConvLayer(nf, nf, norm_type=norm_type, **kwargs),
                        ConvLayer(nf, nf, norm_type=norm_type, **kwargs),
                        MergeLayer(dense=True))

instead, more succinctly:

@add_kwargs(norm_type=NormType.Batch)
@delegates(ConvLayer)
def DenseResBlock(nf, **kwargs):
    "Resnet block of `nf` features. `conv_kwargs` are passed to `conv_layer`."
    return SequentialEx(ConvLayer(nf, nf, **kwargs), 
                        ConvLayer(nf, nf, **kwargs), 
                        MergeLayer(dense=True))

@kessido
Copy link
Contributor Author

kessido commented Dec 22, 2020

@kessido can you provide an example of why this would be useful? Why do you need to add additional args other than what is provided in delegates or what is in the function that is being delegated to? It would be helpful to understand the use case if you have one in mind! (it sounds like you might from the code above, but I don't want to guess).

For more informative answer, I find that many times there is many repetitions in code with delegations where the default of some variable is changed in the specific function.

Imo adding the overridden default in the definition, like: def DenseResBlock(nf, norm_type=NormType.Batch, **kwargs): is quite good for usability, but the code implementation become messy:

  1. you add a lot of not so DRY: a=a,b=b,c=c,d=d, or in this case norm_type=norm_type twice.
  2. you sometimes forget to pass the overridden defaults, as it is easy to forget that just writing **kwargs is not sufficient to pass the arguments.

I think that for code readability adding it as a attribute is ok (in the top of the function), and the code itself become more clean with less places to mess up the overridden defaults.

@josiahls
Copy link

josiahls commented Mar 17, 2022

I need a similar behavior. I think a simpler example would be helpful. If you do:

class A():
    def __init__(a=1,b=2):pass
@delegates(A)
def some_fn(**kwargs): print(kwargs)
some_fn()
>>> {}

When I would expect:

class A():
    def __init__(a=1,b=2):pass
@delegates(A)
def some_fn(**kwargs): print(kwargs)
some_fn()
>>> {a:1,b:2}

Why do I want to do this? A more complicated example if when I want to use a call_parse cli function to initialize a class:

class A():
    def __init__(
            a:int=1, # I want the docments for the parameter `a` to show up for `run_a` as well.
            b:int=2, # I want the docments for the parameter `b` to show up for `run_a` as well.
            log:str='info' # I want the docments for the parameter `log` to show up for `run_a` as well.
        ):pass


@call_parse
@delegates(A)
def run_a(**kwargs):
    logging.basicConfig(**default_logging())
    _logger.setLevel(kwargs['log'])
    
    a=A(**kwargs)    
    with a: a.loop()

Its pretty annoying to need to type the description and type hints twice to be able to do this, and delegates almost gets me there.
I will admit, most of the defaults in kwargs could still be handled by the A class when initializing, and the few that I need to access before initializing A can just be handled via: kwargs.get('log','INFO') or something. So its mostly dry with exceptions.

@jph00
Copy link
Contributor

jph00 commented Mar 29, 2022

I have a feeling some or all of this was recently added by @muellerzr ...

@muellerzr
Copy link
Contributor

muellerzr commented Mar 29, 2022

Yes. If you use delegates any docment-defined definitions it will get picked up and shown with show_doc @josiahls

(Unsure about the print(**kwargs), but at the minimum documentation trickles)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants