padertorch icon indicating copy to clipboard operation
padertorch copied to clipboard

Configurable: Support positional only arguments

Open boeddeker opened this issue 3 years ago • 5 comments

At the moment is any callable a configurable, when it supports key value arguments. While this is verbose and most factories and classes are supported, there are a few we don't support.

Maybe the most relevant example for padertorch is torch.nn.Sequential(*args). To use this class, at the moment we have to use a wrapper around that class. A native support would be nice.

In a small group, we discussed this offline, but haven't found the solution, that we want to realize.

First priority is, that the implementation is no breaking change, so all examples that follow, won't break current configs.

Here some examples, how it could be realized:

First, lets assume, we have these factories:

def foo(*numbers):
    ...
def bar(a, b, \):  # positional only, like operator.add
    ...

and we want to call them as

foo(1, 2)
bar(1, 2)

now follow some ideas, how the config could look like:

1. Reserved keyword 'args', 'factory_args' or '*'

{'factory': 'foo', 'args': [1, 2]}
{'factory': 'bar', 'args': [1, 2]}
{'factory': 'foo', '*': [1, 2]}
{'factory': 'bar', '*': [1, 2]}

2. Signature check with "assignment"

Ignore that the arguments is a positional only in the config and simply do an assignment style in the config. In the implementation, we then to the mapping to the positional argument.

{'factory': 'foo', 'numbers': [1, 2]}
{'factory': 'bar', a=1, b=2]}
{'factory': 'foo', '*numbers': [1, 2]}
{'factory': 'bar', a=1, b=2]}

3. Lisp style

The factory can be a list, the first argument is then the function, while the others are the arguments:

{'factory': ['foo', 1, 2]}
{'factory': ['foo', 1, 2]}

My opinion

The 2. is nice for positional only arguments like operator.add. But this rarely happen in practice, because it was only supported for C and C++ functions until py37 (PEP 570). For *args I don't like it. It looks strange and is not robust against renaming.

For the first my favorite key would be args, but it could happen, that someone uses args as normal keyword:

threading.Thread(group=None, target=None, name=None, args=(), kwargs={}, *, daemon=None)
scipy.optimize.minimize_scalar(fun, bracket=None, bounds=None, args=(), method='brent', tol=None, options=None)[source]

We didn't find a relevant example, but it would be better to prevent the conflict.

At the moment I am unsure, if I like {'factory': ['foo', 1, 2]} or {'factory': 'foo', '*'=[1, 2]} more.

boeddeker avatar Sep 15 '20 16:09 boeddeker

I would prefer something like *args, because there won't be any naming conflicts and I do not see why the renaming problem is worse for *args than it is for args.

jensheit avatar Sep 16 '20 06:09 jensheit

That's my view on the three ideas:

1. Reserved keyword

I clearly see the naming problem here. I would start the reserved keyword with something that is usually not used for argument names, like *args or just *, where * might be confusing at first. This is my favorite variant for now.

2. signature check with assignment

This is robust against reordering of arguments, though not against renaming. As this requires to inspect the signature, I think it is too complicated.

3. lisp style

I don't like this style because it changes the type of 'factory' depending on if you provide arguments as positional ars or as keyword args. If you have code like this in your finalize_dogmatic_config, the code changes whether you have positional arguments or not:

def finalize_dogmatic_config(cls, config):
    # Without positional args
    if config['factory'] == some_class:
        config['...'] = 'some default for that class'

    # With positional args. This line looks like there are multiple factories which I find confusing
    if config['factory'][0] == some_class:
        ...

    # Add an argument here
    if isinstance(config['factory'], list):
        config['factory'].append('new_arg')
    else:
        config['factory'] = [config['factory'], 'new_arg']

thequilo avatar Sep 16 '20 07:09 thequilo

3. lisp style

@thequilo That is a good point against this. Now I am also against this.

1 Reserved keyword

Now I am also for this variant.

Maybe another point to consider is the cli. python -m ... with 'trainer.*=[1,2]' (or python -m ... with 'trainer.*args=[1,2]'). The * could be interpreted as wildcard. But this might be a small drawback, args will not often be used.

Another thing is, that we cannot partially overwrite args. This is already the case for lists in the config. But I have no good idea, how this could be solved.

boeddeker avatar Sep 17 '20 14:09 boeddeker

I would definitively prefer the *args version since * is quiet confusing.

jensheit avatar Sep 17 '20 14:09 jensheit

There were proposals to support partial list updates from the command line during the config rework discussion. But nothing got fully implemented. And for expansion, we just have to enclose everything in single quotes

thequilo avatar Sep 17 '20 16:09 thequilo