Fix bug with config_path when using save(config, save_dc_types=True)

[lebrice]

Fixes #276

• Raises a warning whenever the config default is a dict that isn't one of the options in the subgroup values
• Tries to use the closest matching subgroup in that case.

[github-actions]

Result of Benchmark Tests

Benchmark	Min	Max	Mean	Mean on Repo HEAD
test/test_performance.py::test_import_performance	0.02	0.02	0.02 +- 0.00	0.02 +- 0.00
test/test_performance.py::test_parse_performance	0.01	0.01	0.01 +- 0.00	0.02 +- 0.00
test/test_performance.py::test_serialization_performance[.yaml]	0.01	0.01	0.01 +- 0.00	0.02 +- 0.00
test/test_performance.py::test_serialization_performance[.json]	0.00	0.00	0.00 +- 0.00	0.00 +- 0.00
test/test_performance.py::test_serialization_performance[.pkl]	0.00	0.00	0.00 +- 0.00	0.00 +- 0.00

[anivegesana]

Hey @lebrice, is there a status update on this change? It would be nice for me to be able to use the official release of simple_parsing again instead of using this branch.

[lebrice]

Hey @lebrice, is there a status update on this change? It would be nice for me to be able to use the official release of simple_parsing again instead of using this branch.

Hey there! Apologies, no updates on this atm. I'll try to get some work done soon. In the meantime, feel free to make your own PR based on this if you want to help speed up the process :)

[anivegesana]

No problem. It certainly is not urgent. I just want to make sure that it will eventually get in. I can help separate out parts related to this feature into its own PR, although I took most of it from here.

[lebrice]

No problem. It certainly is not urgent. I just want to make sure that it will eventually get in. I can help separate out parts related to this feature into its own PR, although I took most of it from here.

Hey @anivegesana, I just updated the PR, would you mind testing this with your code and letting me know if anything doesn't work?
Thanks!

[anivegesana]

Hey @lebrice,

Great work putting this together so quickly! I ran our internal unit tests using this PR and found just one thing that used to work on our branch that no longer does with this PR:

from_dict(ConfigType, to_dict(config)) no longer seems to roundtrip. Instead, I get the error AssertionError: FIXME: assuming that the _type_ is in the config dict. if a subgroups is present in ConfigType. This is because to_dict doesn't save the _type_ to the subgroup dicts automatically so from_dict is unable to recover the type. I don't know if there was a setting I needed to pass to do this on this PR, but on the branch that I have been testing on, I overloaded the definition of save_dc_types so that False means only save _type_s for subgroups fields instead of never saving _type_, but feel free to choose another solution and I will use whichever one you choose.
anivegesana@aa0e1a2#diff-e2a4574cba7d6626ce9914c133c3fd13ae7ae8feeb579d1f01dbd7a8a026db38R723-R726
anivegesana@aa0e1a2#diff-eda7f0cd48dfe1a9962aea453aa22fd049f9dd6d1795622fe9fa985ca1b77d56R116-R123

Besides this one wrinkle, everything seems to work perfectly. Excellent work!

[anivegesana]

Hey @lebrice,
Any further progress on this PR and any plans to merge it any time soon?
Thanks!

[lebrice]

Hi @anivegesana , sorry I'm currently at NeurIPS.

I'll try to work on this as soon as I can. Thanks for the reminder and sorry for the delay!

[anivegesana]

No problem. Just wanted to bump it again since there wasn't any update in the last couple of months. Take your time and have fun at NeurIPS!

[lebrice]

Hey @anivegesana does this work?

[anivegesana]

Hey @lebrice, thanks for your quick turn around time and hope you enjoyed NeurIPS!

For some reason, our internal tests are failing on this assertion. When I comment it out, nothing bad seems to happen and the test cases pass. This is because there are some places in our config YAMLs that we wrote:

a: KEY

instead of

a:
    _type_: ValueType

Is this intended? If so, I can update the configs on our side to work with this PR, but bringing it up in case you want to support the former way to specify subgroups that do not need any additional fields.

[anivegesana]

Hey @lebrice, happy new year! Any updates on this?

[lebrice]

Yeah I'm working on it. This subgroups feature is in need of some proper refactoring. I'd be tempted to add your fixes and make a quick patch, but there are some underlying issues with the implementation that seem to warrant a broader refactor of that feature.

[anivegesana]

Hey @lebrice, any updates on the feature refactor?

[lebrice]

Hey @anivegesana, thansk for the reminder. Sorry, no updates as of yet on my end. I'll let you know when I get to this (hopefully in the coming weeks).