search

lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
410 stars 52 forks source link

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

Open lebrice opened 1 year ago

lebrice commented 1 year ago

Fixes #276

github-actions[bot] commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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!

anivegesana commented 9 months ago

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

lebrice commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Hey @anivegesana does this work?