Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using subgroups with config_path leads to a TypeError #276

Open
anivegesana opened this issue Jul 20, 2023 · 12 comments · May be fixed by #284
Open

Using subgroups with config_path leads to a TypeError #276

anivegesana opened this issue Jul 20, 2023 · 12 comments · May be fixed by #284

Comments

@anivegesana
Copy link
Contributor

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
Copy link
Owner

lebrice commented Jul 26, 2023

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? (

)

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

@anivegesana
Copy link
Contributor Author

anivegesana commented Jul 26, 2023

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
Copy link
Contributor Author

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 added a commit that referenced this issue Jul 26, 2023
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
lebrice added a commit that referenced this issue Jul 26, 2023
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@lebrice
Copy link
Owner

lebrice commented Aug 2, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

lebrice commented Aug 8, 2023

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
Copy link
Contributor Author

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

anivegesana added a commit to anivegesana/SimpleParsing that referenced this issue Aug 11, 2023
anivegesana added a commit to anivegesana/SimpleParsing that referenced this issue Aug 11, 2023
@anivegesana
Copy link
Contributor Author

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
Copy link

@lebrice Hi, is there any update ?

@lebrice
Copy link
Owner

lebrice commented Oct 24, 2023

@lebrice Hi, is there any update ?

#284 (comment)

@dorbala
Copy link

dorbala commented Feb 23, 2024

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

@lebrice
Copy link
Owner

lebrice commented Feb 26, 2024

@dorbala

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

#284 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants