Increase (or make configurable) the `max_attempts` of `ConflictResolver`

[Tomiinek]

Describe the bug I would like to use bunch of subgroups. They are all the same but go to different destinations. It produces a lot of conflicts, but it seems to be possible to resolve them. However, when I increase the number of the subgroups to parse, I can run out of the conflict resolution attempts which results in an exception:

simple_parsing.conflicts.ConflictResolutionError: Reached maximum number of attempts (50) while trying to solve the conflicting argument names. This is either a bug, or there is something weird going on with your 
class hierarchy/argument names...                                                                                                                                                                                    
In any case, Please help us by submitting an issue on the Github repo at https://github.com/lebrice/SimpleParsing/issues, or by using the following link: https://github.com/lebrice/SimpleParsing/issues/new?assigne
es=lebrice&labels=bug&template=bug_report.md&title=BUG: ConflictResolutionError

(explanation of what I am doing: I have a model with certain parts such as the encoder, decoder1, decoder2, ... Each of these parts has its own optimizer. The optimizer can be of different types (AdamW, LAMB, SGD, ...) and should be configurable (similarly to the Config in the example below). So for each of these parts, I would like to add something like parser.add_arguments(Config, dest="encoder-optimizer"), parser.add_arguments(Config, dest="decoder1-optimizer"), parser.add_arguments(Config, dest="decoder2-optimizer"), ... )

To Reproduce

from __future__ import annotations
from dataclasses import dataclass
from simple_parsing import ArgumentParser, choice, subgroups
from pathlib import Path

@dataclass
class ModelConfig:
    ...

@dataclass
class ModelAConfig(ModelConfig):
    lr: float = 3e-4
    optimizer: str = "Adam"
    betas: tuple[float, float] = 0.9, 0.999

@dataclass
class ModelBConfig(ModelConfig):
    lr: float = 1e-3
    optimizer: str = "SGD"
    momentum: float = 1.234

@dataclass
class Config:

    # Which model to use
    model: ModelConfig = subgroups(
        {"model_a": ModelAConfig, "model_b": ModelBConfig},
        default=ModelAConfig(),
    )

parser = ArgumentParser()
parser.add_arguments(Config, dest="a")
parser.add_arguments(Config, dest="b")
parser.add_arguments(Config, dest="c")
...
parser.add_arguments(Config, dest="z")
args = parser.parse_args("")

config: Config = args.config

print(config)

Expected behavior Parse all the arguments without the exception. I can resolve the issue by increasing max_attempts here: https://github.com/lebrice/SimpleParsing/blob/545aa469defecba6d30da02199a1ea206740ad23/simple_parsing/conflicts.py#L68

[zhiruiluo]

Hi @Tomiinek, you possiblely need to use nesting of dataclasses instead of subgroups. Correct me if I am wrong, you wanna use ModelAConfig and ModelBConfig at the the same instead of exact one of them. Consider my example for you in the following:

from __future__ import annotations
from dataclasses import dataclass
from simple_parsing import ArgumentParser, parse

@dataclass
class ModelAConfig():
    lr: float = 3e-4
    optimizer: str = "Adam"
    betas: tuple[float, float] = 0.9, 0.999

@dataclass
class ModelBConfig():
    lr: float = 1e-3
    optimizer: str = "SGD"
    momentum: float = 1.234

@dataclass
class Config():
    modelAconfig: ModelAConfig
    modelBconfig: ModelBConfig

# using the ArgumentParser API
parser = ArgumentParser()
parser.add_arguments(Config, dest="config")
args = parser.parse_args()
config: Config = args.config

# or using the parse API
config: Config = parse(Config)

[lebrice]

Hey there @Tomiinek , very interesting issue, thanks for posting!

Hmm I don't think that's the issue @zhiruiluo. The two approaches should behave exactly the same way, regardless of if the conflict is in the dataclass fields or in the calls to add_arguments. Are you saying that with your example the conflicts get resolved without having to increase the maximum number of attempts?

I just reworked the subgroups feature substantially. Could you confirm that this is still happening even with the code of the master branch? It's very interesting that increasing the number of attempts fixes this. Do you think this type of example here should be sufficient to reproduce the issue? If so, how high should the number of subgroups be? If not, then could you give me an idea of your class layout?

[zhiruiluo]

@lebrice, I can confirm that there is no conflict for @Tomiinek example and my example in versions of 0.0.21, 0.0.21.post1, and current master. I can't reproduce the ConflictResolutionError by @Tomiinek's example. There might be something missing.

Hey @Tomiinek, Could you provide more details of how to reproduce your issue, such as simple_parsing version, python version, or give real code that you worked on?

[Tomiinek]

Thank you guys for looking into this.

Correct me if I am wrong, you wanna use ModelAConfig and ModelBConfig at the the same instead of exact one of them. Consider my example for you in the following:

I think that the example does not fit my needs

Do you think this type of example here should be sufficient to reproduce the issue? If so, how high should the number of subgroups be? If not, then could you give me an idea of your class layout?

Sure, I will post a meaningful reproducible example, but please let's resolve https://github.com/lebrice/SimpleParsing/issues/191#issuecomment-1378417720 first since I am not sure I am actually able to use subgroups :sweat_smile:

[Tomiinek]

I am not experiencing the issue from simple-parsing===0.0.21.post1.post20-gd0fdcf7, closing.

[janvainer]

@lebrice Hello, I bumped into this issue too with the latest version 1.1.3. Increasing max attempts in the package to 500 solves the issue for me. I am using quite large configs and 3 nesting layers. It stopped working for me after adding sufficiently many options. It would be awesome if this param could be set in some global defaults or in the parsing constructor :D

[lebrice]

Hello @janvainer ! A fix was merged here: https://github.com/lebrice/SimpleParsing/pull/272

Hopefully this helps. I'll make a new release soon.

[janvainer]

Hello, thanks alot - yes this helps! I installed the tip of the master for now :)