working sol

Author: tcapelle

Diff for: simple_parsing/parsing.py

@@ -387,14 +387,10 @@ def set_defaults(self, config_path: str | Path | None = None, **kwargs: Any) ->
         if config_path:
             defaults = read_file(config_path)
             if self.nested_mode == NestedMode.WITHOUT_ROOT and len(self._wrappers) == 1:
-                # The file should have the same format as the command-line args, e.g. contain the
-                # fields of the 'root' dataclass directly (e.g. "foo: 123"), rather a dict with
-                # "config: foo: 123" where foo is a field of the root dataclass at dest 'config'.
-                # Therefore, we add the prefix back here.
-                defaults = {self._wrappers[0].dest: defaults}
-                # We also assume that the kwargs are passed as foo=123
-                kwargs = {self._wrappers[0].dest: kwargs}
-            # Also include the values from **kwargs.
+                # The file should have the same format as the command-line args
+                wrapper = self._wrappers[0]
+                defaults = {wrapper.dest: defaults}
+                kwargs = {wrapper.dest: kwargs}
             kwargs = dict_union(defaults, kwargs)
 
         # The kwargs that are set in the dataclasses, rather than on the namespace.
@@ -640,7 +636,7 @@ def _resolve_subgroups(
             # config_path=self.config_path,
             # NOTE: We disallow abbreviations for subgroups for now. This prevents potential issues
             # for example if you have —a_or_b and A has a field —a then it will error out if you
-            # pass —a=1 because 1 isn’t a choice for the a_or_b argument (because --a matches it
+            # pass —a=1 because 1 isn't a choice for the a_or_b argument (because --a matches it
             # with the abbreviation feature turned on).
             allow_abbrev=False,
         )
@@ -827,6 +823,8 @@ def _instantiate_dataclasses(
         argparse.Namespace
             The transformed namespace with the instances set at their
             corresponding destinations.
+            Also keeps whatever arguments were added in the traditional fashion,
+            i.e. with `parser.add_argument(...)`.
         """
         constructor_arguments = constructor_arguments.copy()
         # FIXME: There's a bug here happening with the `ALWAYS_MERGE` case: The namespace has the
@@ -1157,5 +1155,45 @@ def _create_dataclass_instance(
         else:
             logger.debug(f"All fields for {wrapper.dest} were either at their default, or None.")
             return None
+            
+    # Handle subgroup fields
+    subgroup_fields = {f for f in wrapper.fields if f.is_subgroup}
+    if subgroup_fields:
+        # Create a copy of constructor args to avoid modifying the original
+        filtered_args = constructor_args.copy()
+        
+        # Remove _type_ field if present at top level
+        filtered_args.pop("_type_", None)
+        
+        # For each subgroup field, check if we have parameters that belong to its default type
+        for field_wrapper in subgroup_fields:
+            default_key = field_wrapper.subgroup_default
+            if default_key is not None and default_key is not dataclasses.MISSING:
+                choices = field_wrapper.subgroup_choices
+                default_factory = choices[default_key]
+                if callable(default_factory):
+                    default_type = default_factory
+                else:
+                    default_type = type(default_factory)
+                
+                # Get fields of the default type
+                default_subgroup_fields = {f.name for f in dataclasses.fields(default_type)}
+                
+                # Find which fields in the input match fields in the default subgroup
+                matching_fields = {name: filtered_args[name] for name in list(filtered_args.keys()) 
+                                if name in default_subgroup_fields}
+                
+                if matching_fields:
+                    # Create an instance of the default type with the matching fields
+                    subgroup_instance = default_type(**matching_fields)
+                    filtered_args[field_wrapper.name] = subgroup_instance
+                    
+                    # Remove handled fields
+                    for name in matching_fields:
+                        filtered_args.pop(name, None)
+        
+        # Use the filtered args to create the instance
+        constructor_args = filtered_args
+    
     logger.debug(f"Calling constructor: {constructor}(**{constructor_args})")
     return constructor(**constructor_args)

Diff for: simple_parsing/wrappers/dataclass_wrapper.py

@@ -294,24 +294,66 @@ def set_default(self, value: DataclassT | dict | None):
         self._default = value
         if field_default_values is None:
             return
-        unknown_names = set(field_default_values)
+            
+        # First try to handle any subgroup fields
+        subgroup_fields = {f for f in self.fields if f.is_subgroup}
+        remaining_fields = field_default_values.copy()  # Work with a copy to track what's been handled
+        
+        for field_wrapper in subgroup_fields:
+            # Get the default subgroup type from the choices
+            default_key = field_wrapper.subgroup_default
+            if default_key is not None and default_key is not dataclasses.MISSING:
+                choices = field_wrapper.subgroup_choices
+                default_factory = choices[default_key]
+                if callable(default_factory):
+                    default_type = default_factory
+                else:
+                    default_type = type(default_factory)
+                
+                # Get fields of the default type
+                default_subgroup_fields = {f.name for f in dataclasses.fields(default_type)}
+                
+                # Find which fields in the input match fields in the default subgroup
+                matching_fields = {name: remaining_fields[name] for name in list(remaining_fields.keys()) 
+                                if name in default_subgroup_fields}
+                
+                if matching_fields:
+                    # Create the nested structure for the subgroup
+                    subgroup_dict = {
+                        field_wrapper.name: {
+                            "_type_": default_key,
+                            **matching_fields
+                        }
+                    }
+                    # Set this as the default for this field
+                    field_wrapper.set_default(subgroup_dict[field_wrapper.name])
+                    
+                    # Remove handled fields
+                    for name in matching_fields:
+                        remaining_fields.pop(name, None)
+
+        # Now handle any remaining regular fields
         for field_wrapper in self.fields:
-            if field_wrapper.name not in field_default_values:
+            if field_wrapper.name not in remaining_fields:
                 continue
-            # Manually set the default value for this argument.
-            field_default_value = field_default_values[field_wrapper.name]
-            field_wrapper.set_default(field_default_value)
-            unknown_names.remove(field_wrapper.name)
+            if field_wrapper.is_subgroup:
+                continue
+            # Set default for regular field
+            field_wrapper.set_default(remaining_fields[field_wrapper.name])
+            remaining_fields.pop(field_wrapper.name)
+
+        # Handle nested dataclass fields
         for nested_dataclass_wrapper in self._children:
-            if nested_dataclass_wrapper.name not in field_default_values:
+            if nested_dataclass_wrapper.name not in remaining_fields:
                 continue
-            field_default_value = field_default_values[nested_dataclass_wrapper.name]
-            nested_dataclass_wrapper.set_default(field_default_value)
-            unknown_names.remove(nested_dataclass_wrapper.name)
-        unknown_names.discard("_type_")
-        if unknown_names:
+            nested_dataclass_wrapper.set_default(remaining_fields[nested_dataclass_wrapper.name])
+            remaining_fields.pop(nested_dataclass_wrapper.name)
+
+        # Check for any unhandled fields
+        remaining_fields.pop("_type_", None)  # Remove _type_ if present as it's handled separately
+        if remaining_fields:
             raise RuntimeError(
-                f"{sorted(unknown_names)} are not fields of {self.dataclass} at path {self.dest!r}!"
+                f"{sorted(remaining_fields.keys())} are not fields of {self.dataclass} at path {self.dest!r}!"
             )
 
     @property

Diff for: simple_parsing/wrappers/field_wrapper.py

@@ -719,12 +719,13 @@ def default(self) -> Any:
         if it has a default value
         """
 
-        if self._default is not None:
+        if self.is_subgroup:
+            # For subgroups, always use the subgroup_default to maintain consistency
+            default = self.subgroup_default
+        elif self._default is not None:
             # If a default value was set manually from the outside (e.g. from the DataclassWrapper)
             # then use that value.
             default = self._default
-        elif self.is_subgroup:
-            default = self.subgroup_default
         elif any(
             parent_default not in (None, argparse.SUPPRESS)
             for parent_default in self.parent.defaults

Diff for: test/test_subgroup_minimal.yaml

@@ -1 +1,2 @@
+_type_: type_a
 model_a_param: test

Diff for: test/test_subgroups.py

@@ -964,7 +964,8 @@ class TrainConfig(TestSetup):
     # Create a config file
     config_path = Path(__file__).parent / "test_subgroup_minimal.yaml"
     config = {
-        "model_a_param": "test"  # This should work but currently fails
+        "_type_": "type_a",  # Specify we want to use ModelTypeA
+        "model_a_param": "test"  # Set the parameter
     }
     with config_path.open('w') as f:
         yaml.dump(config, f)
@@ -980,8 +981,8 @@ class TrainConfig(TestSetup):
     # This should work the same way as CLI args
     config_from_file = parse(
         TrainConfig,
-        args=shlex.split(f"--config_path {config_path}"),
-        add_config_path_arg=True,
+        config_path=config_path,
+        args=[],  # Pass empty list to prevent pytest args from being parsed
     )
 
     # These assertions should pass but currently fail because the config file parameters