SimpleParsing
SimpleParsing copied to clipboard
lebrice
Fix bug with config_path when using `save(config, save_dc_types=True)`
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.
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 |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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 @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_parsingagain 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 :)
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.
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!
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.
https://github.com/anivegesana/SimpleParsing/commit/aa0e1a2fda4228199bc3d8ded0686c805778ec90#diff-e2a4574cba7d6626ce9914c133c3fd13ae7ae8feeb579d1f01dbd7a8a026db38R723-R726
https://github.com/anivegesana/SimpleParsing/commit/aa0e1a2fda4228199bc3d8ded0686c805778ec90#diff-eda7f0cd48dfe1a9962aea453aa22fd049f9dd6d1795622fe9fa985ca1b77d56R116-R123
Besides this one wrinkle, everything seems to work perfectly. Excellent work!
Hey @lebrice, Any further progress on this PR and any plans to merge it any time soon? Thanks!
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!
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!