lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
384 stars 46 forks source link

Increasing max_attempts or make configurable #240

Closed JonasFrey96 closed 12 months ago

JonasFrey96 commented 1 year ago

Hello,

For my current application, the max_attempts of 50 are not sufficient. Is it possible to make this configurable or increase it to 100?

Best Jonas

lebrice commented 1 year ago

Hello there @JonasFrey96 , thanks for posting!

Hmmm, that's quite surprising to me. Would you mind sharing a little bit of information about what your config hierarchy looks like? I'm thinking that it's most likely a bug in simple-parsing that is causing your config hierarchy to take more than 50 steps.

JonasFrey96 commented 1 year ago

Hi @lebrice , thanks for your response. The general setup is as follows:

@dataclass
class ExperimentParams(Serializable):
    @dataclass
    class GeneralParams:
         @dataclass
        class FusionNetParams:
            output_channels: int = 1
            apply_sigmoid: List[bool] = field(default_factory=lambda: [True])

        fusion_net: FusionNetParams = FusionNetParams()
   general: GeneralParams = GeneralParams()

I am also using some lists within the paramter files

        @dataclass
        class TargetLayerParams:
            name: str
        aux_layers: List[any] = field(
            default_factory=lambda TargetLayerParams=TargetLayerParams: [
                TargetLayerParams(name="a"),
                TargetLayerParams(name="b"),
                TargetLayerParams(name="c"),
            ]
        )

The total config file is 270 lines of parameters. I ran into this issue after adding a lot of data classes. I hope this information helps.

Best Jonas

JonasFrey96 commented 1 year ago

Hi, @lebrice do you have any updates?

lebrice commented 1 year ago

Hi @JonasFrey96, sorry for the inactivity on this.

Putting aside for a moment my concerns with the complexity of your setup (:stuck_out_tongue: ) I think a first solution would be pretty easy to do: the constant (50) could be made into an attribute or a module-level MAX_ATTEMPTS variable, that you could then easily modify in your script before you call the argument parser's parse_args.

I don't think this should be a constructor argument to the ArgumentParser, seeing as it's already got way too many arguments, and I don't want to give the impression to the users that this value is something they would need to increase in regular use-cases.

Would you potentially be interested in making a PR for this change?

JonasFrey96 commented 1 year ago

Hi, I hope this PR helps. I implemented it via a property. Best Jonas