Using subgroups with config_path leads to a TypeError

[anivegesana]

Describe the bug
When using subgroups with config_path, a type error occurs when trying to resolve which subgroup the config file should belong to. Adding a _type_ or subgroup key attribute automatically to all such entries and instantiating dictionaries that are assigned to subgroups inside of config files is a potential fix.

To Reproduce

from dataclasses import dataclass
from simple_parsing import subgroups, ArgumentParser
from simple_parsing.wrappers.field_wrapper import (
    ArgumentGenerationMode,
    DashVariant,
    NestedMode,
)

@dataclass
class A:
    a: int = 0

@dataclass
class B1(A):
    b: float = 1

@dataclass
class B2(A):
    b: float = 2

@dataclass
class C:
    c: A = subgroups(
        {
            "b1": B1,
            "b2": B2,
        },
        default="b1"
    )

parser = ArgumentParser(
    argument_generation_mode=ArgumentGenerationMode.NESTED,
    nested_mode=NestedMode.WITHOUT_ROOT,
    add_option_string_dash_variants=DashVariant.UNDERSCORE_AND_DASH,
    config_path="config.yaml",
)
parser.add_arguments(C, dest="config")
args = parser.parse_args()
print(args)

c:
  _type_: __main__.B2
  a: 0
  b: 1

Expected behavior
The subgroup entries to be read into the ArgumentParser and able to be modified via command line.

$ python run.py
C(c=B2(a=0,b=1))
$ python run.py --c.a=1
C(c=B2(a=1,b=1))
$ python run.py --c=b1
C(c=B1(a=0,b=1))

Actual behavior
A clear and concise description of what is happening.

$ python run.py
Traceback (most recent call last):
  File "run.py", line 39, in <module>
    args = parser.parse_args()
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/argparse.py", line 1768, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 333, in parse_known_args
    self._preprocessing(args=args, namespace=namespace)
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 523, in _preprocessing
    wrapped_dataclasses, chosen_subgroups = self._resolve_subgroups(
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 703, in _resolve_subgroups
    dataclass_type = subgroup_field.field.metadata["subgroup_dataclass_types"][
TypeError: unhashable type: 'dict'

Desktop (please complete the following information):

• Version: 0.1.3
• Python version: 3.8.10

[lebrice]

Hey there @anivegesana , thanks for posting!

I'll have to look into this bug more closely, but this seems to be related to these lines here: https://github.com/lebrice/SimpleParsing/blob/master/simple_parsing/helpers/serialization/serializable.py#L793-L799.

Just to check, was this config created with the save_dc_types argument to the to_dict function? (

SimpleParsing/simple_parsing/helpers/serialization/serializable.py

Line 691 in cd6fd81

save_dc_types: bool = False,

)

I'll try to look into it, thanks again for posting!

[anivegesana]

Hey @lebrice, thanks for looking into this!

I actually didn't know whether to file this as a feature request or as a bug report since it is a little bit of both, and I think I might have confused you with an incomplete description of what I wanted/expected.

Parsing of subgroups is working perfectly for me and loading configs from external files is working perfectly for me as well. The problem is that if I load a configuration that uses subgroups from an external file, with or without save_dc_types enabled, I run into either an AssertionError if save_dc_types is disabled or the TypeError from above if it is enabled. It works if I replace the dictionary with the subgroup name, but this is less than ideal since I have no way of making modifications to the arguments within the subgroup. This is the bug part.

Traceback (most recent call last):
  File "run.py", line 39, in <module>
    args = parser.parse_args()
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/argparse.py", line 1768, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 333, in parse_known_args
    self._preprocessing(args=args, namespace=namespace)
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 523, in _preprocessing
    wrapped_dataclasses, chosen_subgroups = self._resolve_subgroups(
  File "/home/anirudhvegesana/anaconda3/envs/py38/lib/python3.8/site-packages/simple_parsing/parsing.py", line 651, in _resolve_subgroups
    assert argument_options["default"] is subgroup_field.subgroup_default
AssertionError

In either case, there is no way is specify which subgroup to choose from the config file. This part is more of a feature request. I am fine with if the subgroup is chosen via the _type_ key used by save_dc_types or a new key, like _subgroup_, that is made for this purpose, but I think that adding a feature like this is the most promising way to fix the aforementioned bug.

[anivegesana]

Since the subgroup selection is not stored directly, I don't expect it to be easy to save the subgroups to configuration files via dump_yaml, but I think having a way to specify them from a file is still convenient regardless.

I can open separate issue for subgroup selection via file if these two ideas turn out to be very different from each other, or if you have other ideas about how to accomplish this.

[lebrice]

Thanks a lot @anivegesana , your explanation really helped me to better undestand the issue.

I've been playing around with this a bit, and I think it's more of a bug than a feature request. I don't think that the loading of subgroups from config files is working correctly. I'll hopefully keep you posted on this, please do ping me here if I don't reply soon-ish (within say a week)

[anivegesana]

Hey @lebrice,

You told me to check in within a week. How is it going and how far along have you gotten in figuring out why config_path and subgroups don't play well with each other?

[lebrice]

Hey @anivegesana , thanks for checking in! I've gotten one part of it to work in #284, the _type_ values are correctly being used to choose the type of dataclass to create, and aren't being passed to the constructor anymore (which was causing a TypeError on my end).

As for the rest of it (the AssertionError and TypeError you mention here), I'm not done exploring the issue yet, but it seems to be caused by the assumptions I had when I implemented the subgroups feature:

• The fields are first parsed as regular string arguments with a "choices" corresponding to the keys of the subgroup dict
• Then, the arguments are added for the selected dataclass, and a new round of parsing happens

The issue seems to be with the first assumption, because I'm assuming that the field is first a simple str field, it's a problem for it to already have a default corresponding to a dict.

I'm not done digging on this, but here's a little overview of what I've found so far.

[anivegesana]

Ok. Good to hear that you are making progress on the issue! Hopefully the other half of the problem isn't that challenging.

[anivegesana]

Hey @lebrice, I was somewhat blocking on this issue, so I pieced together parts of #284 to create something that was good enough for my specific issue. Continue working on a permanent fix at your leisure. I attached the commit that I am using for my project above.

[3outeille]

@lebrice Hi, is there any update ?

[lebrice]

@lebrice Hi, is there any update ?

#284 (comment)

[dorbala]

Hey @lebrice @anivegesana - is this finally fixed? This would be a much needed improvement for me too.

[lebrice]

@dorbala

Hey @lebrice @anivegesana - is this finally fixed? This would be a much needed improvement for me too.

#284 (comment)