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

Fix subgroup #343

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix subgroup #343

wants to merge 4 commits into from

Conversation

tcapelle
Copy link

@tcapelle tcapelle commented Feb 26, 2025

This is a standing issue fix: #331

We successfully fixed the issue where subgroups didn't work correctly when parameters were provided through config files. The core problem was that when subgroup parameters were specified in a YAML file, they
couldn't be properly associated with their parent subgroup type.

The Fix

The solution involved two main components:

  1. Smart field association for config files:
    - Added code to detect when fields in a config file match fields in a subgroup's default type
    - When matching fields are found, they're automatically associated with the correct subgroup
    - This happens in both dataclass_wrapper.py and parsing.py when processing configs
  2. Improved callable type resolution:
    - Enhanced how we handle functions and functools.partial objects in subgroups
    - Added robust resolution of return type annotations to better support nested class definitions
    - Fixed edge cases with string annotations by evaluating them in the correct scope

Before vs After

Before:

# This would fail with "RuntimeError: ['model_a_param'] are not fields of <class 'TrainConfig'>"
model_a_param: test

After:

Both approaches now work equivalently:

CLI args

  simple_parsing.parse(TrainConfig, args=['--model_a_param', 'test'])

Config file

simple_parsing.parse(TrainConfig, config_path='config.yaml')  # where config.yaml contains 'model_a_param: test'

The fix preserves backward compatibility while adding the ability to specify subgroup parameters directly in config files without explicit nesting, making the API more intuitive and consistent.

Mostly authored by Claude Code 3.7

This commit fixes two issues:
1. Makes subgroups work properly when values are provided through config files.
   - Recognizes when fields in a config belong to a default subgroup type
   - Properly associates fields with the correct subgroup

2. Improves subgroup callable handling:
   - Better support for functions/callables that return dataclasses
   - Resolves function return type annotations more reliably
   - Fixes tests with nested class definitions

🤖 Generated with Claude Code
Co-Authored-By: Claude <[email protected]>
@lebrice
Copy link
Owner

lebrice commented Feb 26, 2025

Hey there @tcapelle , thanks a lot for this!

I'm going to take a look at the code and I'll get back to you. Also, do you mind if I push some changes to your branch?

@tcapelle
Copy link
Author

go ahead! thanks for looking at this.

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 this pull request may close these issues.

Bug with subgroups and config file
2 participants