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

lebrice · 2023-08-03T22:39:08Z

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 · 2023-08-03T22:39:50Z

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 · 2023-10-23T19:13:23Z

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 · 2023-10-23T21:04:53Z

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 · 2023-10-24T13:09:59Z

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 · 2023-10-27T20:13:56Z

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 · 2023-10-27T20:42:55Z

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 · 2023-12-15T21:52:07Z

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

lebrice · 2023-12-16T00:02:43Z

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 · 2023-12-16T00:30:18Z

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 · 2023-12-21T16:28:37Z

Hey @anivegesana does this work?

anivegesana · 2023-12-21T19:18:32Z

simple_parsing/parsing.py

+                    assert (
+                        argument_options["default"] is subgroup_field.subgroup_default
+                    ), argument_options["default"]


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.

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

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.

Hey @lebrice, any updates on the feature refactor?

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).

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice mentioned this pull request Aug 8, 2023

Using subgroups with config_path leads to a TypeError #276

Open

lebrice marked this pull request as ready for review October 27, 2023 19:50

lebrice force-pushed the lebrice/config_type_key_bug branch 2 times, most recently from 4b2a774 to 4b8f1a3 Compare October 27, 2023 20:11

lebrice changed the title ~~WIP: Fix bug with config_path when using save(config, save_dc_types=True)~~ Fix bug with config_path when using save(config, save_dc_types=True) Oct 27, 2023

lebrice force-pushed the lebrice/config_type_key_bug branch from 4b8f1a3 to 1bd0587 Compare December 21, 2023 15:40

anivegesana reviewed Dec 21, 2023

View reviewed changes

lebrice and others added 13 commits January 31, 2024 14:47

Add test to reproduce a bug with _type_ keys

dd6fcb1

Signed-off-by: Fabrice Normandin <[email protected]>

Fix a bug with parsing with _type_ keys in config

cd87f71

Signed-off-by: Fabrice Normandin <[email protected]>

Fix the typing of the subgroups fn

047b653

Signed-off-by: Fabrice Normandin <[email protected]>

Add test to reproduce the subgroup issue#276

beb8cd0

Signed-off-by: Fabrice Normandin <[email protected]>

Fix issues with * import in helpers module

45bd7c0

Signed-off-by: Fabrice Normandin <[email protected]>

Fix issue with import of subgroups from helpers

28092e0

Signed-off-by: Fabrice Normandin <[email protected]>

Fix non-init fields being passed to DC's __init__

1d1a470

Signed-off-by: Fabrice Normandin <[email protected]>

Deprecate a few unused functions in utils.py

b47f036

Signed-off-by: Fabrice Normandin <[email protected]>

Add temporary fix for the config _type_ issue

8b56786

Signed-off-by: Fabrice Normandin <[email protected]>

Leave a TODO for nested edge case, move fns around

aacecd6

Signed-off-by: Fabrice Normandin <[email protected]>

Remove non-init fields in set_default

234612c

Signed-off-by: Fabrice Normandin <[email protected]>

Add test and use trick from anivegesena

8715594

Signed-off-by: Fabrice Normandin <[email protected]>

Fix broken tests for equivalent_argparse_code

459d3f7

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice and others added 4 commits January 31, 2024 14:52

Fix changed regression files

f4b68fc

Signed-off-by: Fabrice Normandin <[email protected]>

Refactoring subgroup default

f0800c4

Signed-off-by: Fabrice Normandin <[email protected]>

Clarify goal of test with comment

b930cfc

Signed-off-by: Fabrice Normandin <[email protected]>

Move subgroup stuff into a new separate file

ea8a0e1

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice force-pushed the lebrice/config_type_key_bug branch from 7624426 to ea8a0e1 Compare January 31, 2024 19:53

lebrice added 3 commits January 31, 2024 14:59

Fix issues with regression files

a3525f5

Signed-off-by: Fabrice Normandin <[email protected]>

Update regression files slightly

dc26305

Signed-off-by: Fabrice Normandin <[email protected]>

Add note for later

ce02fd1

Signed-off-by: Fabrice Normandin <[email protected]>

lebrice mentioned this pull request Oct 4, 2024

Bug with subgroups and config file #331

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lebrice commented Aug 3, 2023 •

edited

Loading

github-actions bot commented Aug 3, 2023 •

edited

Loading

anivegesana commented Oct 23, 2023

lebrice commented Oct 23, 2023

anivegesana commented Oct 24, 2023

lebrice commented Oct 27, 2023

anivegesana commented Oct 27, 2023

anivegesana commented Dec 15, 2023

lebrice commented Dec 16, 2023

anivegesana commented Dec 16, 2023

lebrice commented Dec 21, 2023

anivegesana Dec 21, 2023

anivegesana Jan 8, 2024

lebrice Jan 31, 2024

anivegesana Apr 9, 2024

lebrice Apr 16, 2024

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

Are you sure you want to change the base?

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

Conversation

lebrice commented Aug 3, 2023 • edited Loading

github-actions bot commented Aug 3, 2023 • edited Loading

Result of Benchmark Tests

anivegesana commented Oct 23, 2023

lebrice commented Oct 23, 2023

anivegesana commented Oct 24, 2023

lebrice commented Oct 27, 2023

anivegesana commented Oct 27, 2023

anivegesana commented Dec 15, 2023

lebrice commented Dec 16, 2023

anivegesana commented Dec 16, 2023

lebrice commented Dec 21, 2023

anivegesana Dec 21, 2023

Choose a reason for hiding this comment

anivegesana Jan 8, 2024

Choose a reason for hiding this comment

lebrice Jan 31, 2024

Choose a reason for hiding this comment

anivegesana Apr 9, 2024

Choose a reason for hiding this comment

lebrice Apr 16, 2024

Choose a reason for hiding this comment

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

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

lebrice commented Aug 3, 2023 •

edited

Loading

github-actions bot commented Aug 3, 2023 •

edited

Loading