-
Notifications
You must be signed in to change notification settings - Fork 16
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
Configurable: Support positional only arguments #61
Comments
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. |
That's my view on the three ideas: 1. Reserved keywordI clearly see the naming problem here. I would start the reserved keyword with something that is usually not used for argument names, like 2. signature check with assignmentThis 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 styleI 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 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'] |
3. lisp style@thequilo That is a good point against this. Now I am also against this. 1 Reserved keywordNow I am also for this variant. Maybe another point to consider is the cli. Another thing is, that we cannot partially overwrite args. This is already the case for lists in the config. |
I would definitively prefer the *args version since * is quiet confusing. |
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 |
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
istorch.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:
and we want to call them as
now follow some ideas, how the config could look like:
1. Reserved keyword 'args', 'factory_args' or '*'
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.
3. Lisp style
The factory can be a list, the first argument is then the function, while the others are the arguments:
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 usesargs
as normal keyword: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.The text was updated successfully, but these errors were encountered: