Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OmegaConf.to_object
: Instantiate structured configs #502OmegaConf.to_object
: Instantiate structured configs #502Changes from 78 commits
15d9abb
c4b042a
fe47c89
c91be99
59ee909
14b75be
3354bad
5cb06ed
a3676e3
c5f5e73
abf8af0
a54cb98
5fdd019
fc49cbd
64b3ed7
41588c1
653a54e
65497ee
1640f8b
7e52b24
8dfd397
68b1f74
5b47049
15324e9
375babf
6422f39
ecc05b1
9d7addc
7136baa
841ac01
fa81a4d
872850b
072e8d8
1953a47
91de025
a4af7f5
efcc93b
f94ecbd
5b51861
7e299cf
19de3b4
84c00ac
488a4b3
bbbb245
37e055f
961b9fd
7f8addb
a10fa5b
6b014ae
4cefc6d
b2a5ab2
d28ae5d
62f34cf
249ac36
0869121
3a47132
46aadbe
c581548
2cf460f
d6e9749
f16ad22
2bf73b0
eb41a37
30550bf
0611c93
32f6c68
1019df6
3fef7f0
0019bca
15a03ea
f3171f2
80284f4
8f17b9a
e1e034a
5045503
29323a4
fe5df1d
a9a05ee
db09880
a17a11f
29a526b
c672c10
672b180
8beb52a
4787e8d
c1d13f8
bc2f610
f1a4270
b51d33f
d12701e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it make more sense to iterate on the content of self and not to pass instance_data at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would work. The values of
self
are subconfig objects that are not yet converted to python primitives (possibly DictConfig or ListConfig, and possibly containing unresolved interpolations). The values ofinstance_data
are already resolved and converted to dict or list or to dataclass/attrclass instances.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data passed in is derived from the content of self, which is why I am asking it.
It feels redundant.
You have custom code here that is handling things, it can resolve interpolations and deal with containers.
am I missing anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true.
Take a look at how
instance_data
is calculated in the calling function (DictConfig._to_content
)... The calculation is nontrivial, and it depends on several things that are defined in the scope of the calling function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the calling function has to perform this calculation anyway (for the case where
structured_config_mode == SCMode.DICT
), I think it makes sense to reuse the calculation by passing ininstance_data
for thestructured_config_mode == SCMode.INSTANTIATE
case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will not apply resolve and enum_to_str to the instantiated objects (but will apply it values in unstructured config containers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, makes sense to me.
So:
enum_to_str=False
andresolve=False
,enum_to_str=False
andresolve=False
, andenum_to_str
andresolve
according to keyword args chosen by the client.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a Structured Config may contain an unstructured DictConfig, we could consider inheriting the resolve flag for them from the surrounding call, but I am not sure it's worth the added complexity.
in that case, the signature of to_object() will look something like:
An alternative is to just say that everything under a Structured Config is always resolved (and enums are not converted to strings).
I think I prefer the second option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Me too. I'll get started on the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c672c10.