Add test and use trick from anivegesena

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Author: lebrice

.pre-commit-config.yaml

@@ -28,10 +28,10 @@ repos:
 
   - repo: https://github.com/charliermarsh/ruff-pre-commit
     # Ruff version.
-    rev: 'v0.0.261'
+    rev: "v0.0.261"
     hooks:
       - id: ruff
-        args: ['--line-length', '99', '--fix']
+        args: ["--line-length", "99", "--fix"]
         require_serial: true
 
   # python code formatting
@@ -44,7 +44,7 @@ repos:
 
   # python docstring formatting
   - repo: https://github.com/myint/docformatter
-    rev: v1.5.1
+    rev: v1.7.5
     hooks:
       - id: docformatter
         args: [--in-place, --wrap-summaries=99, --wrap-descriptions=99]
@@ -65,7 +65,6 @@ repos:
       - id: nbstripout
         require_serial: true
 
-
   # md formatting
   - repo: https://github.com/executablebooks/mdformat
     rev: 0.7.16
@@ -80,7 +79,6 @@ repos:
           # - mdformat-black
         require_serial: true
 
-
   # word spelling linter
   - repo: https://github.com/codespell-project/codespell
     rev: v2.2.2

simple_parsing/helpers/serialization/serializable.py

@@ -208,7 +208,8 @@ def __init_subclass__(
                 if parent in SerializableMixin.subclasses and parent is not SerializableMixin:
                     decode_into_subclasses = parent.decode_into_subclasses
                     logger.debug(
-                        f"Parent class {parent} has decode_into_subclasses = {decode_into_subclasses}"
+                        f"Parent class {parent} has decode_into_subclasses = "
+                        f"{decode_into_subclasses}"
                     )
                     break
 
@@ -220,7 +221,10 @@ def __init_subclass__(
         register_decoding_fn(cls, cls.from_dict)
 
     def to_dict(
-        self, dict_factory: type[dict] = dict, recurse: bool = True, save_dc_types: bool = False
+        self,
+        dict_factory: type[dict] = dict,
+        recurse: bool = True,
+        save_dc_types: bool | int = False,
     ) -> dict:
         """Serializes this dataclass to a dict.
 
@@ -462,7 +466,8 @@ class SimpleSerializable(SerializableMixin, decode_into_subclasses=True):
 def get_serializable_dataclass_types_from_forward_ref(
     forward_ref: type, serializable_base_class: type[S] = SerializableMixin
 ) -> list[type[S]]:
-    """Gets all the subclasses of `serializable_base_class` that have the same name as the argument of this forward reference annotation."""
+    """Gets all the subclasses of `serializable_base_class` that have the same name as the argument
+    of this forward reference annotation."""
     arg = get_forward_arg(forward_ref)
     potential_classes: list[type] = []
     for serializable_class in serializable_base_class.subclasses:
@@ -595,6 +600,7 @@ def loads_yaml(
 
 def read_file(path: str | Path) -> dict:
     """Returns the contents of the given file as a dictionary.
+
     Uses the right function depending on `path.suffix`:
     {
         ".yml": yaml.safe_load,
@@ -613,7 +619,7 @@ def save(
     obj: Any,
     path: str | Path,
     format: FormatExtension | None = None,
-    save_dc_types: bool = False,
+    save_dc_types: bool | int = False,
     **kwargs,
 ) -> None:
     """Save the given dataclass or dictionary to the given file."""
@@ -704,7 +710,7 @@ def to_dict(
     dc: DataclassT,
     dict_factory: type[dict] = dict,
     recurse: bool = True,
-    save_dc_types: bool = False,
+    save_dc_types: bool | int = False,
 ) -> dict:
     """Serializes this dataclass to a dict.
 
@@ -736,6 +742,11 @@ def to_dict(
         else:
             d[DC_TYPE_KEY] = module + "." + class_name
 
+    # Decrement save_dc_types if it is an int, so that we only save the type of the subgroups
+    # dataclass, not all dataclasses recursively.
+    if save_dc_types is not True and save_dc_types > 0:
+        save_dc_types -= 1
+
     for f in fields(dc):
         name = f.name
         value = getattr(dc, name)
@@ -763,7 +774,8 @@ def to_dict(
                 encoded = encoding_fn(value)
             except Exception as e:
                 logger.error(
-                    f"Unable to encode value {value} of type {type(value)}! Leaving it as-is. (exception: {e})"
+                    f"Unable to encode value {value} of type {type(value)}! Leaving it as-is. "
+                    f"(exception: {e})"
                 )
                 encoded = value
         d[name] = encoded
@@ -931,13 +943,11 @@ def is_dataclass_or_optional_dataclass_type(t: type) -> bool:
 
 @functools.lru_cache(maxsize=None)
 def _locate(path: str) -> Any:
-    """
-    COPIED FROM Hydra:
-    https://github.com/facebookresearch/hydra/blob/f8940600d0ab5c695961ad83abd042ffe9458caf/hydra/_internal/utils.py#L614
+    """COPIED FROM Hydra: https://github.com/facebookresearch/hydra/blob/f8940600d0ab5c695961ad83ab
+    d042ffe9458caf/hydra/_internal/utils.py#L614.
 
-    Locate an object by name or dotted path, importing as necessary.
-    This is similar to the pydoc function `locate`, except that it checks for
-    the module from the given path from back to front.
+    Locate an object by name or dotted path, importing as necessary. This is similar to the pydoc
+    function `locate`, except that it checks for the module from the given path from back to front.
     """
     if path == "":
         raise ImportError("Empty path")
@@ -972,7 +982,8 @@ def _locate(path: str) -> Any:
                 except ModuleNotFoundError as exc_import:
                     raise ImportError(
                         f"Error loading '{path}':\n{repr(exc_import)}"
-                        + f"\nAre you sure that '{part}' is importable from module '{parent_dotpath}'?"
+                        + f"\nAre you sure that '{part}' is importable from module "
+                        f"'{parent_dotpath}'?"
                     ) from exc_import
                 except Exception as exc_import:
                     raise ImportError(

simple_parsing/helpers/subgroups.py

@@ -9,8 +9,9 @@
 from typing import Any, Callable, Mapping, TypeVar, Union
 
 from typing_extensions import TypeAlias
+from simple_parsing.helpers.serialization.serializable import to_dict
 
-from simple_parsing.utils import DataclassT, is_dataclass_instance, is_dataclass_type
+from simple_parsing.utils import Dataclass, DataclassT, is_dataclass_instance, is_dataclass_type
 
 logger = get_logger(__name__)
 
@@ -80,7 +81,11 @@ def subgroups(
     metadata["subgroup_default"] = default
     metadata["subgroup_dataclass_types"] = {}
 
-    subgroup_dataclass_types: dict[Key, type[DataclassT]] = {}
+    # Custom encoding function that will add the _type_ key with the subgroup dataclass type.
+    # Using an int here means that only to the subgroup dataclass.
+    kwargs.setdefault("encoding_fn", functools.partial(to_dict, save_dc_types=1))
+
+    subgroup_dataclass_types: dict[Key, type[Dataclass]] = {}
     choices = subgroups.keys()
 
     # NOTE: Perhaps we could raise a warning if the default_factory is a Lambda, since we have to
@@ -198,7 +203,8 @@ def _get_dataclass_type_from_callable(
             f"{dataclass_fn!r}, because it doesn't have a return type annotation, and we don't "
             f"want to call it just to figure out what it produces."
         )
-        # NOTE: recurse here, so it also works with `partial(partial(...))` and `partial(some_function)`
+        # NOTE: recurse here, so it also works with `partial(partial(...))` and
+        # `partial(some_function)`
         # Recurse, so this also works with partial(partial(...)) (idk why you'd do that though.)
 
     if isinstance(signature.return_annotation, str):
@@ -241,7 +247,8 @@ def _get_dataclass_type_from_callable(
 def is_lambda(obj: Any) -> bool:
     """Returns True if the given object is a lambda expression.
 
-    Taken from https://stackoverflow.com/questions/3655842/how-can-i-test-whether-a-variable-holds-a-lambda
+    Taken from
+    https://stackoverflow.com/questions/3655842/how-can-i-test-whether-a-variable-holds-a-lambda
     """
     LAMBDA = lambda: 0  # noqa: E731
     return isinstance(obj, type(LAMBDA)) and obj.__name__ == LAMBDA.__name__

test/test_subgroups.py

@@ -11,11 +11,13 @@
 from typing import Callable, TypeVar
 
 import pytest
-from simple_parsing.helpers.serialization import save
 from pytest_regressions.file_regression import FileRegressionFixture
 from typing_extensions import Annotated
 
+from simple_parsing.utils import Dataclass
+from simple_parsing.helpers.serialization import save
 from simple_parsing import ArgumentParser, parse, subgroups
+from simple_parsing.helpers.serialization.serializable import from_dict, to_dict
 from simple_parsing.wrappers.field_wrapper import ArgumentGenerationMode, NestedMode
 
 from .test_choice import Color
@@ -190,7 +192,7 @@ def test_parse(dataclass_type: type[TestClass], args: str, expected: TestClass):
 
 
 def test_subgroup_choice_is_saved_on_namespace():
-    """test for https://github.com/lebrice/SimpleParsing/issues/139
+    """Test for https://github.com/lebrice/SimpleParsing/issues/139.
 
     Need to save the chosen subgroup name somewhere on the args.
     """
@@ -278,7 +280,9 @@ def test_subgroup_default_factory_needs_to_be_value_in_dict():
 
 def test_lambdas_dont_return_same_instance():
     """Slightly unrelated, but I just want to check if lambda expressions return the same object
-    instance when a default factory looks like `lambda: A()`. If so, then I won't encourage this.
+    instance when a default factory looks like `lambda: A()`.
+
+    If so, then I won't encourage this.
     """
 
     @dataclass
@@ -294,8 +298,7 @@ class Config(TestSetup):
 
 def test_partials_new_args_overwrite_set_values():
     """Double-check that functools.partial overwrites the keywords that are stored when it is
-    created with the ones that are passed when calling it.
-    """
+    created with the ones that are passed when calling it."""
     # just to avoid the test passing if I were to hard-code the same value as the default by
     # accident.
     default_a = A().a
@@ -425,7 +428,10 @@ class Foo(TestSetup):
     marks=pytest.mark.xfail(
         strict=True,
         raises=NotImplementedError,
-        reason="Lambda expressions aren't allowed in the subgroup dict or default_factory at the moment.",
+        reason=(
+            "Lambda expressions aren't allowed in the subgroup dict or default_factory at the "
+            "moment."
+        ),
     ),
 )
 
@@ -440,7 +446,8 @@ class Foo(TestSetup):
     ],
 )
 def test_other_default_factories(a_factory: Callable[[], A], b_factory: Callable[[], B]):
-    """Test using other kinds of default factories (i.e. functools.partial or lambda expressions)"""
+    """Test using other kinds of default factories (i.e. functools.partial or lambda
+    expressions)"""
 
     @dataclass
     class Foo(TestSetup):
@@ -596,7 +603,7 @@ class Config(TestSetup):
 
 
 def test_destination_substring_of_other_destination_issue191():
-    """Test for https://github.com/lebrice/SimpleParsing/issues/191"""
+    """Test for https://github.com/lebrice/SimpleParsing/issues/191."""
 
     parser = ArgumentParser()
     parser.add_arguments(Config, dest="config")
@@ -783,11 +790,12 @@ def test_help(
 #     ModelConfig = _ModelConfig()
 #     SmallModel = _ModelConfig(num_layers=1, hidden_dim=32)
 #     BigModel = _ModelConfig(num_layers=32, hidden_dim=128)
-
-#     @dataclasses.dataclass
-#     class Config(TestSetup):
-#         model: Model = subgroups({"small": SmallModel, "big": BigModel}, default_factory=SmallModel)
-
+#
+#    @dataclasses.dataclass
+#    class Config(TestSetup):
+#        model: Model = subgroups({"small": SmallModel, "big": BigModel},
+#                                 default_factory=SmallModel)
+#
 #     assert Config.setup().model == SmallModel()
 #     # Hopefully this illustrates why Annotated aren't exactly great:
 #     # At runtime, they are basically the same as the original dataclass when called.
@@ -803,7 +811,7 @@ def test_help(
 
 @pytest.mark.parametrize("frozen", [True, False])
 def test_nested_subgroups(frozen: bool):
-    """Assert that #160 is fixed: https://github.com/lebrice/SimpleParsing/issues/160"""
+    """Assert that #160 is fixed: https://github.com/lebrice/SimpleParsing/issues/160."""
 
     @dataclass(frozen=frozen)
     class FooConfig:
@@ -978,3 +986,17 @@ def test_parse_with_config_file_with_different_subgroup(
 
     save(value_in_config, config_path, save_dc_types=True)
     assert parse(A1OrA2, config_path=config_path, args=args) == expected
+
+
+@pytest.mark.parametrize(
+    "value",
+    [
+        A1OrA2(),
+        A1OrA2(a=A2(a_val=2)),
+    ],
+)
+def test_roundtrip(value: Dataclass):
+    """Test to reproduce
+    https://github.com/lebrice/SimpleParsing/pull/284#issuecomment-1783490388."""
+    assert from_dict(type(value), to_dict(value)) == value
+    assert to_dict(from_dict(type(value), to_dict(value))) == to_dict(value)