Add singleton option #3013
Add singleton option #3013
Conversation
facebook-github-bot
added
the
CLA Signed
|
Seems like the test and doc github workflows aren't configured to run on PRs from forks, mind also adding |
| @@ -22,6 +22,7 @@ class _Keys(str, Enum): | |||
| RECURSIVE = "_recursive_" | |||
| ARGS = "_args_" | |||
| PARTIAL = "_partial_" | |||
| SINGLETON = "_singleton_" | |||
There was a problem hiding this comment.
nit: "singleton_id" feels more descriptive that it should be a string value, not bool
| @@ -239,6 +246,7 @@ def instantiate( | |||
| if is_structured_config(config) or isinstance(config, (dict, list)): | |||
| config = OmegaConf.structured(config, flags={"allow_objects": True}) | |||
|
|
|||
| singleton_registry = {} | |||
There was a problem hiding this comment.
Did you consider making the registry more global so they can be saved across instantiate calls? Would be interested to know if anyone has strong opinions on which behavior is preferred
| if is_singleton: | ||
| singleton_name = instantiate_node(node.get(_Keys.SINGLETON), | ||
| singleton_registry=singleton_registry) | ||
| if singleton_name in singleton_registry: |
There was a problem hiding this comment.
Can/should we enforce singleton_name is a string?
| singleton_name = instantiate_node(node.get(_Keys.SINGLETON), | ||
| singleton_registry=singleton_registry) | ||
| if singleton_name in singleton_registry: | ||
| return singleton_registry[singleton_name] |
There was a problem hiding this comment.
I'm concerned this could get confusing if two different dicts have the same singleton_name and it's not totally obvious which will be used. Since this primarily seems to be wanted with omegaconf resolution, the dicts should be identical - what do you think of storing the original node in singleton_registry and doing an extra check that this node's fields also match the original?
Motivation
Adds a singleton option similar to that proposed in #1393
Instead of
_singleton_: truea unique name is given to each singleton,_singleton_: "my_singletons_name"this allows the config file to beresolvedand still have singletons work.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Can add tests that nested singletons are working correctly. Not sure how to set up tests to see if this will break any interactions with the other flags like
recursiveorpartialRelated Issues and PRs
resolves #1393
Todo
Add tests and documentation