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 _target_ to be a callable object itself in recursive construction #1017

Closed
ppwwyyxx opened this issue Sep 26, 2020 · 6 comments · Fixed by #1019
Closed
Labels
enhancement Enhanvement request
Milestone

Comments

@ppwwyyxx
Copy link

🚀 Feature Request

Now _target_ has to be the full name of the callable object, supporting _target_ to be a callable object itself in recursive construction would be very helpful.

Motivation

In some situation there are some advantages using the callable directly:

  • the name is often too long, such as library.module_name.submodule_name.class
  • user writing a new class/function in code, and want to swap this class/function with the existing one in config.

Pitch

In https://github.com/facebookresearch/hydra/blob/6ac5a6270530ff4acf76248a01e11c3cc4278847/examples/instantiate/object_recursive/my_app.py, be able to use the following instead:

class MyNewDriver()
...

@hydra.main(config_name="config")
def my_app(cfg: DictConfig) -> None:
    cfg = OmegaConf.to_container(cfg)
    cfg['car']['driver']['_target_'] = MyNewDriver
    car: Car = instantiate(cfg['car'])
    car.drive()
@ppwwyyxx ppwwyyxx added the enhancement Enhanvement request label Sep 26, 2020
@omry omry added this to the 1.1.0 milestone Sep 26, 2020
@omry
Copy link
Collaborator

omry commented Sep 26, 2020

One thing that would work well with this is the ability to override the _target_ similarly to the ability to override _recursive_ in the #986.

car: Car = instantiate(cfg.car, driver={"_target_": MyNewDriver})
# or the equivalent:
car: Car = instantiate(cfg.car, **{"driver": {"_target_": MyNewDriver}})

I will look into supporting both features, although the callable name can be obtained via name:

car: Car = instantiate(cfg.car, driver={"_target_": MyNewDriver.__name__})

@omry
Copy link
Collaborator

omry commented Sep 26, 2020

One this that I am not sure about here is in what scenario this is preferred over config composition.
In your example, it's likely that MyNewDriver will require different parameters than Driver, and now your code will need to manually override multiple things.
The approach Hydra is taking to this problem is to use config groups and select the right option which will contain the correct config from the start - eliminating the need to manipulate it at program run time.

Do you have an example where building the config at runtime is somehow preferred to allowing Hydra to compose it?
keep in mind that for tests etc you can use the Compose API.

@omry
Copy link
Collaborator

omry commented Sep 27, 2020

Upon an attempt to implement this I realized this is in conflict with an existing feature:
if a type is a dataclass:

@dataclass
class Foo:
  x : float = 10

and you assign it to an OmegaConf container

cfg = OmegaConf.create()
cfg.foo = Foo

The result is equivalent to assigning an object of that type:

cfg = OmegaConf.create()
cfg.foo = Foo()

This is only true for dataclasses. however as they become more pervasive I am concerned that supporting this feature will cause confusion to people.
requiring a string solves that problem.

Obtaining a string you can use from a class or callable is straight forward and you don't need to type the whole thing:

def get_full_name(type_or_callable : Any) -> str:
  return f"{type_or_callable.__module__}.{type_or_callable.__name__}"

I think supporting this feature will - in the long term - cause more problems than it's solving.

@omry
Copy link
Collaborator

omry commented Sep 28, 2020

I think I was able to work around it in #1019 by converting input types to string (as in get_full_name() above) before we engage OmegaConf.
for context, the reason I use OmegaConf there is to enable interpolation support during instantiation.

cfg = OmegaConf.create({"a": 10, "b": 20})
instantiate(cfg, b="${a}") # a == 10, b == a

This kind of support extends to both the config object (which can be a regular dict or a DictConfig) and also to the parameter overrides. That's why it was pretty hard to support type there (as OmegaConf has it's own interpretation of dataclass types).

@ppwwyyxx
Copy link
Author

Thanks, I think that will work for all common scenarios. Note that however the extra round trip from callable -> string -> callable may not reliably work for everything, for example it's not obvious whether it works for lambda, list, class defined inside another class/function etc. These corner cases are probably not necessary, though.

@omry
Copy link
Collaborator

omry commented Sep 30, 2020

Thanks for pointing it out.
I think it's actually good to require that people only use targets that can be described in a config. using inline lambdas as targets means that it will be impossible to achieve the same logic with a pure config approach.

If there are specific cases where this can work but does not we can look at them and try to improve.

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