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

Recursive instantiation #566

Closed
omry opened this issue May 4, 2020 · 9 comments · Fixed by #989
Closed

Recursive instantiation #566

omry opened this issue May 4, 2020 · 9 comments · Fixed by #989
Labels
enhancement Enhanvement request In progress Work in progress
Milestone

Comments

@omry
Copy link
Collaborator

omry commented May 4, 2020

this would go well with a change to the config definition of an instantiated object:

Current

model:
  cls: foo.bar.ResNet
  params:
    num_layers: 50

New

model:
  _target_: model.ResNet
  num_layers: 50

Once this is implemented, we can describe recursive instantiation like:

trainer:
  _target_: trainer.Trainer
  batch_size: 32
  model:
    _target_: model.ResNet
    num_layers: 50

An remaining open question is how to support passthrough for nested objects.

Currently, passthrough can override individual values:

trainer = instantiate(cfg.trainer, batch_size=64)

a possible approach is for nested objects is to use a dictionary:

trainer = instantiate(cfg.trainer, batch_size=64, overrides={"trainer.model.num_layers": 50})
@shagunsodhani
Copy link
Contributor

Is the New style intended to replace the current one?

@omry
Copy link
Collaborator Author

omry commented Aug 11, 2020

Not entirely. If you don't own the the dataclass you will not be able to add the type field to it.

@shagunsodhani
Copy link
Contributor

Okay my original concern was if we could keep around the params namespace but if both the styles can be used, that request is moot.

@omry
Copy link
Collaborator Author

omry commented Aug 11, 2020

we may want to deprecate it in favor of

args:
 - positional_1
 - positional_1
kwargs:
  named_1 : 1
  named_2 : 2

@shagunsodhani
Copy link
Contributor

we may want to deprecate it in favor of

args:
 - positional_1
 - positional_1
kwargs:
  named_1 : 1
  named_2 : 2

Looks good!

@omry omry mentioned this issue Sep 20, 2020
7 tasks
@omry omry added In progress Work in progress and removed help wanted Community help is wanted labels Sep 20, 2020
@omry omry changed the title RFC: Recursive instantiation Recursive instantiation Sep 21, 2020
@omry
Copy link
Collaborator Author

omry commented Sep 23, 2020

This is coming in Hydra 1.1.
I have a question for people who liked this issue (indicating that they would like to have this supported):

I would like to make recursive instantiation the default, but this is a breaking change for people that are actually interested in it because right now they are getting the nested config as a config and they need to manually call instantiate on it.
Assuming that there will be an alternative way to instantiate non recursively, is such a breaking change acceptable to folks here?

Like this comment to indicate yes and dislike it to indicate no.
Feel free to followup with a discussion.

@goens
Copy link

goens commented Sep 28, 2020

I second having this be the default behavior. For me personally it is an acceptable breaking change, since it allows to have portable code (that does not depend on Hydra) and use Hydra on top for configuring. Currently, calling instantiate manually breaks the modularity since we can only instantiate via Hydra this way (and we would like to be able to do both).

@omry
Copy link
Collaborator Author

omry commented Sep 28, 2020

Can you explain more how instantiate behaving one way or another interacts with your ability to do things outside of Hydra?

@omry omry closed this as completed in #989 Sep 28, 2020
@omry
Copy link
Collaborator Author

omry commented Sep 28, 2020

This is now in Hydra master branch.
Feedback welcome.

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

Successfully merging a pull request may close this issue.

3 participants