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

[Feature Request] Support for Var Args and Positional Only Args #1432

Closed
monney opened this issue Feb 26, 2021 · 6 comments · Fixed by #1470
Closed

[Feature Request] Support for Var Args and Positional Only Args #1432

monney opened this issue Feb 26, 2021 · 6 comments · Fixed by #1470
Labels
enhancement Enhanvement request
Milestone

Comments

@monney
Copy link
Contributor

monney commented Feb 26, 2021

🚀 Feature Request

Hi Omry!
In a yaml file under the _target_ I'd like the ability to pass things to functions and constructors which accept var args or positional only arguments.

Motivation

Is your feature request related to a problem? Please describe.

Functions like np.random.randn are currently uncallable. Some Objects are unconstructable.

Pitch

Describe the solution you'd like
I'd like to be able to write something like this in a yaml file

positional_only_ob:
  _target_: PositionalOnlyArgs
  args:
    foo: 15 #positional only arg
  bar: 20 # regular arg

for var args the syntax might be something like this:

var_args_ob:
  _target_: VarArgs
  args: #var args
   - 1 
   - 2
   - 3
  bar: 20 # regular arg

Describe alternatives you've considered

  • We could make it so we use _args_ instead of args.
  • We could use a different keyword of kwargs, and pass by position by default.
  • You could always use the proposed positional only args syntax since names will be ignored anyways
  • You could always use the proposed var args syntax (I think)

Are you willing to open a pull request? (See CONTRIBUTING)
Yes

Additional context

I've tried on master (1.1.dev5)

As a side note, this feature is necessary to support instantiation of tuples and sets, but that is a separate feature request I'll likely make once if this becomes supported.

@monney monney added the enhancement Enhanvement request label Feb 26, 2021
@omry
Copy link
Collaborator

omry commented Feb 26, 2021

Hi @monney.
There is already a feature request for it (#808).

I am open to an attempt to add this, but as I said in the other feature request the surface area of instantiate is very large.
in other words, it's already one of the most complicated pieces of code in Hydra.
The bar for such a contribution will be really high in terms of testing.

A few things to consider:

  1. There is already a way to send positional arguments (although I think are currently incorrectly being passed to recursive instantiations as well, which should be fixed - they they should only be passed to the first instantiation)
  2. There should be a way to pass positional arguments as passthrough to nested config nodes.
  3. I don't mind deprecating the current form of positional arguments in favor of a more flexible one:
# example passing 1,2,3 as positional to the first node, and 4,5,6 to the nested one.
instantiate(cfg.node, {"_args_": [1,2,3], foo={"_args_" : [4,5,6]}})
  1. I think the best thing to do if both the config and the passthrough has _args_ is to have the passthrough override full replace the list in the config.
  2. We should consider this in the context of interpolations. What is supported here?
  3. Whatever we come up with should be well documented.

As you can see, there is a lot going on here.

Regarding support for instantiating tuples and sets: This is supported, but not with recursive instantiation.

In [17]: utils.instantiate({"_target_" : "builtins.tuple"}, [1,2,3])
Out[17]: (1, 2, 3)

In [18]: utils.instantiate({"_target_" : "builtins.set"}, [1,2,3])
Out[18]: {1, 2, 3}

@omry omry added this to the Hydra 1.2.0 milestone Feb 27, 2021
@omry
Copy link
Collaborator

omry commented Feb 27, 2021

Will take a look at it for 1.2.

It may get into 1.1 if you can deliver a solution before it gets released.

@monney
Copy link
Contributor Author

monney commented Feb 27, 2021

I agree with the 1.2 timeline, given the complexity, it will take me some time.

A, few questions, currently the tuple instantiation and "args" doesn't work through yaml properties, correct? Only through passing it in the instantiate function?

Could you clarify what you mean by passthrough to nested nodes?

Thanks

@omry
Copy link
Collaborator

omry commented Feb 27, 2021

A, few questions, currently the tuple instantiation and "args" doesn't work through yaml properties, correct? Only through passing it in the instantiate function?

instantiation of builtins works.
passing positional arguments only works via the instantiate function right now (and I think it does not work well if you are doing recursive instantiation).

Could you clarify what you mean by passthrough to nested nodes?

You mean "There should be a way to pass positional arguments as passthrough to nested config nodes" ?

This if we use _args_ as the holder of positional arguments, this should be made to work:

# example passing 1,2,3 as positional to the first node, and 4,5,6 to the nested one.
instantiate(cfg.node, {"_args_": [1,2,3], foo={"_args_" : [4,5,6]}})

@monney
Copy link
Contributor Author

monney commented Feb 27, 2021

For builtins I can't get tuple at least to work with yaml only, since 3.8 because it requires positional only.

For passthrough I had never tried that, I'll keep that in mind for implementation and tests.

I'll start going over the new instantiate code to get a gauge on things.

@omry
Copy link
Collaborator

omry commented Feb 27, 2021

I have a pretty big PR changing instantiation that I am going to merge soon. see the outstanding pull requests.

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

Successfully merging a pull request may close this issue.

2 participants