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 |
| 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
| 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
There was a problem hiding this comment.
I though about adding it to hydra global but that caused a circular reference in the modules. It could be passed in as an optional parameter to instantiate so that someone has the option to re-use it
| 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?
There was a problem hiding this comment.
that makes sense, will update
| 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?
There was a problem hiding this comment.
ya that is a bit concerning and tricky to catch, checking the dicts are the same feels wrong but would be good to check to give atleast a warning.
|
Due to some of the issues I also added an alternate method that I think is a bit more robust: #3025 will move to the feature request to get feedback on which approach would fit better |
|
I made something similar here #3063. Do you know why this wasn't accepted? |
|
Thank you for the PR and for exploring this direction. I am going to close this for now. The PR is incomplete as an implementation proposal: it needs tests, documentation, and a news fragment. More importantly, the API shape and semantics for shared object identity during If you would like to keep pursuing this, please start a design discussion for your proposal first. Once the API and semantics are agreed on, an implementation PR will be much easier to evaluate. |
5 participants
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