Repository: lebrice/SimpleParsing

Issue:
Bug when mixing subgroups and "regular" dataclasses
When mixing both, e.g.

```python
@dataclass
class Config:
    person: Person = subgroups({
        "bob": Bob, "alice": Alice,
    }, default=Bob)

parser = ArgumentParser()
parser.add_arguments(Config, dest="config")
parser.add_arguments(Foo, dest="foo")

args = parser.parse_args()

```
Raises a
RuntimeError("namespace shouldnt already have a "foo" attribute!")

Discussion:

Patch:
diff --git a/simple_parsing/parsing.py b/simple_parsing/parsing.py
index bb0dee1..4af37e0 100644
--- a/simple_parsing/parsing.py
+++ b/simple_parsing/parsing.py
@@ -2,12 +2,24 @@
 @author: Fabrice Normandin
 """
 import argparse
+import dataclasses
 import sys
 from argparse import SUPPRESS, Action, HelpFormatter, Namespace, _, _HelpAction
 from collections import defaultdict
 from functools import partial
 from logging import getLogger
-from typing import Any, Callable, Dict, List, Sequence, Type, Union, overload
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    Optional,
+    Sequence,
+    Type,
+    TypeVar,
+    Union,
+    overload,
+)
 
 from . import utils
 from .conflicts import ConflictResolution, ConflictResolver
@@ -251,6 +263,10 @@ class ArgumentParser(argparse.ArgumentParser):
             # make sure that args are mutable
             args = list(args)
 
+        # default Namespace built from parser defaults
+        if namespace is None:
+            namespace = Namespace()
+
         self._preprocessing()
 
         logger.debug(f"Parser {id(self)} is parsing args: {args}, namespace: {namespace}")
@@ -265,33 +281,62 @@ class ArgumentParser(argparse.ArgumentParser):
                 argument_generation_mode=self.argument_generation_mode,
                 nested_mode=self.nested_mode,
             )
+            if not hasattr(namespace, "subgroups"):
+                namespace.subgroups = {}
+            if not hasattr(parsed_args, "subgroups"):
+                parsed_args.subgroups = {}
 
             for dest, subgroup_dict in self.subgroups.items():
                 value = getattr(parsed_args, dest)
+
+                def _get_wrapper_for_dest(dest: str) -> FieldWrapper:
+                    """Retrieve the FieldWrapper from self._wrappers that has the given dest."""
+                    for wrapper in self._wrappers:
+                        for field_wrapper in wrapper.fields:
+                            if field_wrapper.dest == dest:
+                                return field_wrapper
+                    raise ValueError(f"No FieldWrapper found for dest {dest}")
+
+                field_wrapper = _get_wrapper_for_dest(dest)
+
                 logger.debug(f"Chosen value for subgroup {dest}: {value}")
                 # The prefix should be 'thing' instead of 'bob.thing'.
                 # TODO: This needs to be tested more, in particular with argument conflicts.
                 _, _, parent_dest = dest.rpartition(".")
                 prefix = parent_dest + "."
+                default = None
+                chosen_subgroup: str
                 if isinstance(value, str):
-                    chosen_class = subgroup_dict[value]
-                    parser.add_arguments(
-                        chosen_class,
-                        dest=dest,
-                        prefix=prefix,
-                    )
+                    chosen_subgroup = value
+                    chosen_class = subgroup_dict[chosen_subgroup]
                 else:
-                    default = value
+                    # The value is not the subgroup name.
                     chosen_class = type(value)
-                    # prefix = f"{dest}."
-                    parser.add_arguments(
-                        chosen_class,
-                        dest=dest,
-                        prefix=prefix,
-                        default=default,
-                    )
-
-            parsed_args, unparsed_args = parser.parse_known_args(args, namespace)
+                    default = value
+                    if chosen_class in subgroup_dict.values():
+                        chosen_subgroup = [
+                            k for k, v in subgroup_dict.items() if v == chosen_class
+                        ].pop()
+                    else:
+                        # A dynamic kind of default value?
+                        raise RuntimeError(
+                            f"Don't yet know how to detect which subgroup in {subgroup_dict} was "
+                            f"chosen, if the default value is {value} and the field default is "
+                            f"{field_wrapper.field.default}"
+                        )
+                # prefix = f"{dest}."
+                parsed_args.subgroups[dest] = chosen_subgroup
+                namespace.subgroups[dest] = chosen_subgroup
+
+                parser.add_arguments(
+                    chosen_class,
+                    dest=dest,
+                    prefix=prefix,
+                    default=default,
+                )
+            # NOTE: Passing the `parsed_args` as the namespace to the child, so that it treats its
+            # values as the defaults.
+            parsed_args, unparsed_args = parser.parse_known_args(args, namespace=parsed_args)
 
         if unparsed_args and self._subparsers and attempt_to_reorder:
             logger.warning(
@@ -497,6 +542,33 @@ class ArgumentParser(argparse.ArgumentParser):
         sorted_wrappers: List[DataclassWrapper] = sorted(
             self._wrappers, key=lambda w: w.nesting_level, reverse=True
         )
+        D = TypeVar("D")
+
+        def _create_dataclass_instance(
+            wrapper: DataclassWrapper[D],
+            constructor: Callable[..., D],
+            constructor_args: Dict[str, Dict],
+        ) -> Optional[D]:
+
+            # Check if the dataclass annotation is marked as Optional.
+            # In this case, if no arguments were passed, and the default value is None, then return
+            # None.
+            if wrapper.optional and wrapper.default is None:
+                for field_wrapper in wrapper.fields:
+                    arg_value = constructor_args[field_wrapper.name]
+                    default_value = field_wrapper.default
+                    logger.debug(
+                        f"field {field_wrapper.name}, arg value: {arg_value}, "
+                        f"default value: {default_value}"
+                    )
+                    if arg_value != default_value:
+                        # Value is not the default value, so an argument must have been passed.
+                        # Break, and return the instance.
+                        break
+                else:
+                    logger.debug(f"All fields for {wrapper.dest} were either default or None.")
+                    return None
+            return constructor(**constructor_args)
 
         for wrapper in sorted_wrappers:
             for destination in wrapper.destinations:
@@ -507,30 +579,9 @@ class ArgumentParser(argparse.ArgumentParser):
                 # the constructor. Might be fine though.
                 constructor = wrapper.dataclass
                 constructor_args = self.constructor_arguments[destination]
-
                 # If the dataclass wrapper is marked as 'optional' and all the
                 # constructor args are None, then the instance is None.
-                # TODO: How to discern the case where all values ARE none, and
-                # the case where the instance is to be None?
-                if wrapper.optional and wrapper.default is None:
-                    all_default_or_none = True
-                    for field_wrapper in wrapper.fields:
-                        arg_value = constructor_args[field_wrapper.name]
-                        default_value = field_wrapper.default
-                        logger.debug(
-                            f"field {field_wrapper.name}, arg value: {arg_value}, default value: {default_value}"
-                        )
-                        if arg_value != default_value:
-                            all_default_or_none = False
-                            break
-                    logger.debug(f"All fields were either default or None: {all_default_or_none}")
-
-                    if all_default_or_none:
-                        instance = None
-                    else:
-                        instance = constructor(**constructor_args)
-                else:
-                    instance = constructor(**constructor_args)
+                instance = _create_dataclass_instance(wrapper, constructor, constructor_args)
 
                 if wrapper.parent is not None:
                     parent_key, attr = utils.split_dest(destination)
@@ -540,30 +591,118 @@ class ArgumentParser(argparse.ArgumentParser):
                     )
                     self.constructor_arguments[parent_key][attr] = instance
 
+                elif not hasattr(parsed_args, destination):
+                    logger.debug(
+                        f"setting attribute '{destination}' on the Namespace "
+                        f"to a value of {instance}"
+                    )
+
+                    setattr(parsed_args, destination, instance)
+                elif not self.subgroups:
+                    existing = getattr(parsed_args, destination)
+                    raise RuntimeError(
+                        f"Namespace should not already have a '{destination}' "
+                        f"attribute!\n"
+                        f"The value would be overwritten:\n"
+                        f"- existing value: {existing}\n"
+                        f"- new value:      {instance}"
+                    )
+                elif not any(wrapper.dest in subgroup_dest for subgroup_dest in self.subgroups):
+                    # the current dataclass wrapper doesn't have anything to do with the subgroups.
+                    existing = getattr(parsed_args, destination)
+                    # This dataclass wrapper doesn't have anything to do with subgroups.
+                    # BUG: The value in the args should be always the same, no?
+
+                    if existing != instance:
+                        logger.warning(
+                            f"Ignoring new parsed value of {instance} for {destination}. \n"
+                            f"Keeping the value that is already in the args: {existing}"
+                        )
+                        # raise RuntimeError(
+                        #     "Why is there already an attribute, if this doesn't have anything to do with subgroups?\n"
+                        #     f"Namespace should not already have a '{destination}' "
+                        #     f"attribute!\n"
+                        #     f"The value would be overwritten:\n"
+                        #     f"- existing value: {existing}\n"
+                        #     f"- new value:      {instance}"
+                        # )
                 else:
-                    # logger.debug(
-                    #     f"setting attribute '{destination}' on the Namespace "
-                    #     f"to a value of {instance}"
-                    # )
-                    # TODO: Do we want to overwrite the value if it's a subgroup choice?
-                    if hasattr(parsed_args, destination):
-                        # It's ok to overwrite the value of a subgroup choice.
-                        # For instance, say its --optimizer_type = "adam", then we don't save this
-                        # value in the namespace. We overwrite it with the value of the config.
-                        # TODO: I guess we could save the value in the namespace, but where?
-                        # NOTE: This is happening here with the parent of a subgroup field, e.g.
-                        # `Bob(thing="foo")`` is to be overwritten with `Bob(thing=Foo(a=123, b=2))`
-                        if any(
-                            field_wrapper.is_subgroup and field_wrapper.dest.startswith(destination)
-                            for field_wrapper in wrapper.fields
-                        ):
-                            pass
+                    # The dataclass wrapper isn't directly for the subgroup: It's for a parent of
+                    # the subgroup choice.
+
+                    existing = getattr(parsed_args, destination)
+                    # It's ok to overwrite the value of a subgroup choice.
+                    # For instance, say its --optimizer_type "adam", then we overwrite the value
+                    # at the 'optimizer' key with the value the dataclass config.
+
+                    # TODO: Detect if this wrapper isn't related (e.g. doesn't have any subgroups.)
+
+                    # NOTE: This is also happening here when the parent of a subgroup field, e.g.
+                    # `Bob(thing="foo")`` is to be overwritten with `Bob(thing=Foo(a=123, b=2))`
+
+                    # Should we just overwrite (set) the attribute on the existing instance?
+                    # or just replace the instance entirely?
+                    for subgroup_dest, subgroup_choices in self.subgroups.items():
+                        # NOTE: These two cases are simpler, but don't occur here: They are
+                        # handled in the FieldWrapper.
+                        subgroup_parent, _, attribute = subgroup_dest.rpartition(".")
+
+                        if wrapper.dest not in subgroup_dest:
+                            # current dataclass wrapper doesn't have anything to do with this
+                            # subgroup. Go to the next subgroup.
+                            continue
+
+                        elif subgroup_parent == destination:
+                            # Replace the attribute that changed on the dataclass instance.
+                            current_value = getattr(existing, attribute)
+                            new_value = getattr(instance, attribute)
+
+                            if (
+                                isinstance(current_value, str)
+                                and current_value in subgroup_choices
+                                and not isinstance(new_value, str)
+                            ):
+                                # Replacing the subgroup name with the dataclass instance.
+                                logger.debug(
+                                    f"Overwriting attribute {attribute} on {existing} from "
+                                    f"{current_value} -> {new_value}."
+                                )
+                                existing = dataclasses.replace(existing, **{attribute: new_value})
+                                setattr(parsed_args, destination, existing)
+                        elif subgroup_dest.startswith(wrapper.dest + "."):
+                            # subgroup overwrites an attribute in the child of the existing
+                            # dataclass.
+                            path_from_parent_to_child = subgroup_dest[len(wrapper.dest + ".") :]
+                            parts = path_from_parent_to_child.split(".")
+
+                            from simple_parsing.utils import (
+                                getattr_recursive,
+                                setattr_recursive,
+                            )
+
+                            existing_config = getattr_recursive(existing, path_from_parent_to_child)
+                            new_config = getattr_recursive(instance, path_from_parent_to_child)
+                            if existing_config == new_config:
+                                continue
+
+                            if (
+                                isinstance(existing_config, str)
+                                and existing_config in subgroup_choices
+                                and not isinstance(new_config, str)
+                            ):
+                                attribute = parts[-1]
+                                # TODO: This won't work if the dataclasses are frozen! We need some
+                                # kind of recursive replace function.
+                                logger.debug(
+                                    f"Overwriting attribute {attribute} on {existing} to {new_config}."
+                                )
+                                setattr_recursive(existing, path_from_parent_to_child, new_config)
+
                         else:
                             raise RuntimeError(
-                                f"Namespace should not already have a '{destination}' "
-                                f"attribute! (namespace: {parsed_args}) "
+                                "Something went wrong while trying to update the value of a \n"
+                                "subgroup in the args! Please report this bug by opening an issue!"
                             )
-                    setattr(parsed_args, destination, instance)
 
                 # TODO: not needed, but might be a good thing to do?
                 # remove the 'args dict' for this child class.
diff --git a/simple_parsing/utils.py b/simple_parsing/utils.py
index be626bc..25bc3c7 100644
--- a/simple_parsing/utils.py
+++ b/simple_parsing/utils.py
@@ -678,10 +678,17 @@ def setattr_recursive(obj: object, attribute_name: str, value: Any):
         setattr_recursive(child_object, ".".join(parts[1:]), value)
 
 
+def getattr_recursive(obj: object, attribute_name: str):
+    if "." not in attribute_name:
+        return getattr(obj, attribute_name)
+    else:
+        child_attr, _, rest_of_attribute_name = attribute_name.partition(".")
+        child_object = getattr(obj, child_attr)
+        return getattr_recursive(child_object, rest_of_attribute_name)
+
+
 def split_dest(destination: str) -> Tuple[str, str]:
-    splits = destination.split(".")
-    parent = ".".join(splits[:-1])
-    attribute_in_parent = splits[-1]
+    parent, _, attribute_in_parent = destination.rpartition(".")
     return parent, attribute_in_parent
 
 
diff --git a/simple_parsing/wrappers/field_wrapper.py b/simple_parsing/wrappers/field_wrapper.py
index cb32457..cd2440c 100644
--- a/simple_parsing/wrappers/field_wrapper.py
+++ b/simple_parsing/wrappers/field_wrapper.py
@@ -1,21 +1,24 @@
 import argparse
-import typing
 import dataclasses
 import inspect
+import sys
+import typing
 from enum import Enum, auto
 from logging import getLogger
-from typing import cast, ClassVar, Any, Optional, List, Type, Dict, Set, Union, Tuple
+from typing import Any, ClassVar, Dict, List, Optional, Set, Tuple, Type, Union, cast
 
 from simple_parsing.help_formatter import TEMPORARY_TOKEN
 
 from .. import docstring, utils
+from .field_metavar import get_metavar
 
 # from ..utils import Dataclass, DataclassType
 from .field_parsing import get_parsing_fn
-from .field_metavar import get_metavar
 from .wrapper import Wrapper
 
 if typing.TYPE_CHECKING:
+    from simple_parsing import ArgumentParser
+
     from .dataclass_wrapper import DataclassWrapper
 
 logger = getLogger(__name__)
@@ -26,12 +29,14 @@ class ArgumentGenerationMode(Enum):
     Enum for argument generation modes.
     """
 
-    # Tries to generate flat arguments, removing the argument destination path when possible.
     FLAT = auto()
-    # Generates arguments with their full destination path.
+    """ Tries to generate flat arguments, removing the argument destination path when possible. """
+
     NESTED = auto()
-    # Generates both the flat and nested arguments.
+    """ Generates arguments with their full destination path. """
+
     BOTH = auto()
+    """ Generates both the flat and nested arguments. """
 
 
 class NestedMode(Enum):
@@ -39,11 +44,14 @@ class NestedMode(Enum):
     Controls how nested arguments are generated.
     """
 
-    # By default, the full destination path is used.
     DEFAULT = auto()
-    # The full destination path is used, but the first level is removed.
-    # Useful because sometimes the first level is uninformative (i.e. 'args').
+    """ By default, the full destination path is used. """
+
     WITHOUT_ROOT = auto()
+    """
+    The full destination path is used, but the first level is removed.
+    Useful because sometimes the first level is uninformative (i.e. 'args').
+    """
 
 
 class DashVariant(Enum):
@@ -191,6 +199,17 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                 f"of parent at key '{parent_dest}' and attribute "
                 f"'{attribute}'"
             )
+            if self.is_subgroup:
+                if not hasattr(namespace, "subgroups"):
+                    namespace.subgroups = {}
+
+                if isinstance(value, str) and value in self.subgroup_choices.keys():
+                    # We've just parsed the name of the chosen subgroup.
+                    # NOTE: There can't be any ambiguity here, since the keys of the dictionary are
+                    # string, and the values are always dataclass types. We don't need to worry
+                    # about having to deal with {"bob": "alice", "alice": "foo"}-type weirdness.
+                    namespace.subgroups[self.dest] = value
+                    logger.info(f"Chosen subgroup for '{self.dest}':  '{value}'")
 
     def get_arg_options(self) -> Dict[str, Any]:
         """Create the `parser.add_arguments` kwargs for this field."""
@@ -237,7 +256,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
             # NOTE: Optional[<something>] is always translated to
             # Union[<something>, NoneType]
             assert type_arguments
-            non_none_types = [t for t in type_arguments if t is not type(None)]
+            non_none_types = [t for t in type_arguments if t is not type(None)]  # noqa: E721
             assert non_none_types
 
             if len(non_none_types) == 1:
@@ -599,7 +618,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
             dashes.extend(additional_dashes)
 
         # remove duplicates by creating a set.
-        option_strings = set(f"{dash}{option}" for dash, option in zip(dashes, options))
+        option_strings = {f"{dash}{option}" for dash, option in zip(dashes, options)}
         # TODO: possibly sort the option strings, if argparse doesn't do it
         # already.
         return list(sorted(option_strings, key=len))
@@ -762,9 +781,11 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                 # NOTE: Here we'd like to convert the fields type to an actual type, in case the
                 # `from __future__ import annotations` feature is used.
                 # This should also resolve most forward references.
-                field_type = utils.get_field_type_from_annotations(
-                    self.parent.dataclass, self.field.name
+                from simple_parsing.annotation_utils.get_field_annotations import (
+                    get_field_type_from_annotations,
                 )
+
+                field_type = get_field_type_from_annotations(self.parent.dataclass, self.field.name)
                 self._type = field_type
 
             if self.is_choice and self.choice_dict:
@@ -872,6 +893,12 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
     def is_subgroup(self) -> bool:
         return "subgroups" in self.field.metadata
 
+    @property
+    def subgroup_choices(self) -> Dict[str, Type]:
+        if not self.is_subgroup:
+            raise RuntimeError(f"Field {self.field} doesn't have subgroups! ")
+        return self.field.metadata["subgroups"]
+
     @property
     def type_arguments(self) -> Optional[Tuple[Type, ...]]:
         return utils.get_type_arguments(self.type)
@@ -896,9 +923,8 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                     for dataclass_type in type_arguments
                 }
 
-    def add_subparsers(self, parser: argparse.ArgumentParser):
+    def add_subparsers(self, parser: "ArgumentParser"):
         assert self.is_subparser
-        from simple_parsing import ArgumentParser  # Just for typing.
 
         # add subparsers for each dataclass type in the field.
         default_value = self.field.default
@@ -910,10 +936,9 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
             title=self.name,
             description=self.help,
             dest=self.dest,
-            parser_class=ArgumentParser,
+            parser_class=type(parser),
             required=(default_value is dataclasses.MISSING),
         )
-        import sys
 
         if sys.version_info[:2] == (3, 6):
             required = add_subparser_kwargs.pop("required")
@@ -930,7 +955,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
             subparser = subparsers.add_parser(subcommand)
             # Just for typing correctness, as we didn't explicitly change
             # the return type of subparsers.add_parser method.)
-            subparser = cast(ArgumentParser, subparser)
+            subparser = cast("ArgumentParser", subparser)
             subparser.add_arguments(dataclass_type, dest=self.dest)
 
     def equivalent_argparse_code(self):


Test patch:
diff --git a/test/conftest.py b/test/conftest.py
index 43a29dc..e627d81 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -74,13 +74,13 @@ from logging import getLogger as get_logger
 @pytest.fixture(scope="session", autouse=True)
 def setup_logging():
     project_logger = get_logger("simple_parsing")
-    project_logger.setLevel(
-        logging.DEBUG
-        if "-vv" in sys.argv
-        else logging.INFO
-        if "-v" in sys.argv
-        else logging.WARNING
-    )
+    # project_logger.setLevel(
+    #     logging.DEBUG
+    #     if "-vv" in sys.argv
+    #     else logging.INFO
+    #     if "-v" in sys.argv
+    #     else logging.WARNING
+    # )
     ch = logging.StreamHandler(stream=sys.stdout)
     ch.setFormatter(
         logging.Formatter(
diff --git a/test/test_conflicts.py b/test/test_conflicts.py
index 7039252..5b756ad 100644
--- a/test/test_conflicts.py
+++ b/test/test_conflicts.py
@@ -1,9 +1,11 @@
 """Tests for weird conflicts.
 """
+import argparse
 from dataclasses import dataclass
+
 from simple_parsing import ArgumentParser
 
-from .testutils import *
+from .testutils import TestSetup, raises
 
 
 def test_arg_and_dataclass_with_same_name(silent):
diff --git a/test/test_default_args.py b/test/test_default_args.py
index 2f4030f..b3e5fcb 100644
--- a/test/test_default_args.py
+++ b/test/test_default_args.py
@@ -1,8 +1,9 @@
+import argparse
 from dataclasses import dataclass
 
 from simple_parsing import ArgumentParser
 
-from .testutils import argparse, raises_missing_required_arg, shlex
+from .testutils import raises_missing_required_arg, shlex
 
 
 def test_no_default_argument(simple_attribute, silent):
diff --git a/test/test_subgroups.py b/test/test_subgroups.py
index 4a8fde8..5c85d1e 100644
--- a/test/test_subgroups.py
+++ b/test/test_subgroups.py
@@ -1,4 +1,5 @@
 import contextlib
+import shlex
 from dataclasses import dataclass, is_dataclass
 from io import StringIO
 from typing import Union
@@ -6,6 +7,7 @@ from typing import Union
 import pytest
 
 from simple_parsing import ArgumentParser, subgroups
+from simple_parsing.wrappers.field_wrapper import ArgumentGenerationMode, NestedMode
 
 from .testutils import TestSetup, raises_missing_required_arg, raises_unrecognized_args
 
@@ -58,7 +60,7 @@ class TestSubgroup:
     def test_subgroup(self):
         parser = ArgumentParser()
         parser.add_arguments(Bob, dest="bob")
-        args = parser.parse_args("--thing foo_thing --thing.a 123".split())
+        args = parser.parse_args(shlex.split("--thing foo_thing --thing.a 123"))
         bob = args.bob
         thing = bob.thing
         assert is_dataclass(thing)
@@ -165,3 +167,112 @@ def test_unrelated_arg_raises_error():
 
     with raises_unrecognized_args("--bblarga"):
         Bob.setup("--first foo --first.a 123 --second foo --second.a 456 --bblarga")
+
+
+@dataclass
+class Person:
+    age: int
+
+
+@dataclass
+class Daniel(Person):
+    """Person named Bob."""
+
+    age: int = 32
+    cool: bool = True
+
+
+@dataclass
+class Alice(Person):
+    """Person named Alice."""
+
+    age: int = 13
+    popular: bool = True
+
+
+@dataclass
+class Config:
+    """Configuration dataclass."""
+
+    person: Person = subgroups({"daniel": Daniel, "alice": Alice}, default=Daniel)
+
+    @dataclass
+    class Config:
+        """Configuration dataclass."""
+
+        person: Person = subgroups({"daniel": Daniel, "alice": Alice}, default=Daniel)
+
+
+@dataclass
+class HigherConfig(TestSetup):
+    """Higher-level config."""
+
+    a: Config = Config(person=Daniel())
+    b: Config = Config(person=Alice())
+
+
+def test_mixing_subgroup_with_regular_dataclass():
+
+    parser = ArgumentParser()
+    parser.add_arguments(Config, dest="config")
+    parser.add_arguments(Foo, dest="foo")
+
+    args = parser.parse_args([])
+    assert args.config == Config(person=Daniel())
+    assert args.foo == Foo()
+
+    # NOTE: Not sure if the parser can safely be reused twice.
+    parser = ArgumentParser()
+    parser.add_arguments(Config, dest="config")
+    parser.add_arguments(Foo, dest="foo")
+    args = parser.parse_args(shlex.split("--person alice --person.age=33 --a 123"))
+    assert args.config == Config(person=Alice(age=33))
+    assert args.foo == Foo(a=123)
+
+
+def test_issue_139():
+    """test for https://github.com/lebrice/SimpleParsing/issues/139
+
+    Need to save the chosen subgroup name somewhere on the args.
+    """
+
+    parser = ArgumentParser()
+    parser.add_arguments(Config, dest="config")
+
+    args = parser.parse_args([])
+    assert args.config == Config(person=Daniel())
+    assert args.subgroups == {"config.person": "daniel"}
+
+
+def test_deeper_nesting_prefixing():
+    """Test that the prefixing mechanism works for deeper nesting of subgroups."""
+
+    assert "--a.person.cool" in HigherConfig.get_help_text(
+        "--help",
+        nested_mode=NestedMode.WITHOUT_ROOT,
+        argument_generation_mode=ArgumentGenerationMode.NESTED,
+    )
+
+    assert "--a.person.popular" in HigherConfig.get_help_text(
+        "--a.person alice --help",
+        nested_mode=NestedMode.WITHOUT_ROOT,
+        argument_generation_mode=ArgumentGenerationMode.NESTED,
+    )
+
+    assert HigherConfig.setup("") == HigherConfig()
+    assert HigherConfig.setup("--a.person alice") == HigherConfig(a=Config(person=Alice()))
+    assert HigherConfig.setup("--b.person daniel --b.person.age 54") == HigherConfig(
+        b=Config(person=Daniel(age=54))
+    )
+
+
+def test_subgroups_dict_in_args():
+    parser = ArgumentParser()
+    parser.add_arguments(HigherConfig, "config")
+
+    args = parser.parse_args([])
+    assert args.config == HigherConfig()
+    assert args.subgroups == {"config.a.person": "daniel", "config.b.person": "alice"}
+
+    args = parser.parse_args(shlex.split("--a.person alice --b.person daniel --b.person.age 54"))
+    assert args.subgroups == {"config.a.person": "alice", "config.b.person": "daniel"}
diff --git a/test/testutils.py b/test/testutils.py
index 3224950..790b908 100644
--- a/test/testutils.py
+++ b/test/testutils.py
@@ -1,4 +1,3 @@
-import argparse
 import shlex
 import string
 import sys
@@ -162,7 +161,13 @@ class TestSetup:
         assert hasattr(args, dest), f"attribute '{dest}' not found in args {args}"
         instance: Dataclass = getattr(args, dest)  # type: ignore
         delattr(args, dest)
-        assert args == argparse.Namespace(), f"Namespace has leftover garbage values: {args}"
+
+        # If there are subgroups, we can allow an extra "subgroups" attribute, otherwise we don't
+        # expect any other arguments.
+        args_dict = vars(args).copy()
+        args_dict.pop("subgroups", None)
+
+        assert not args_dict, f"Namespace has leftover garbage values (besides subgroups): {args}"
 
         instance = cast(Dataclass, instance)
         return instance
@@ -193,6 +198,7 @@ class TestSetup:
         multiple=False,
         conflict_resolution_mode: ConflictResolution = ConflictResolution.AUTO,
         add_option_string_dash_variants=DashVariant.AUTO,
+        **parser_kwargs,
     ) -> str:
         import contextlib
         from io import StringIO
@@ -209,6 +215,7 @@ class TestSetup:
                 argv,
                 conflict_resolution_mode=conflict_resolution_mode,
                 add_option_string_dash_variants=add_option_string_dash_variants,
+                **parser_kwargs,
             )
         s = f.getvalue()
         return s