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

Using config_path with subparsers does not seem to work #241

Open
aliounis opened this issue Apr 10, 2023 · 5 comments · May be fixed by #243
Open

Using config_path with subparsers does not seem to work #241

aliounis opened this issue Apr 10, 2023 · 5 comments · May be fixed by #243

Comments

@aliounis
Copy link

Describe the bug
I am attempting to use subparsers and the config_path option together, however, I cannot seem to get the config path to actually set the defaults. I have tried multiple "paths" in the json file for the config file, posted below, but non seem to work. I've tried to investigate myself and think that the issue might be because the subparser does not copy the defaults for the command attribute (which chooses the subparser).

To Reproduce


import logging

from simple_parsing import ArgumentParser, Serializable


@dataclass
class DoA(Serializable):
    a_config: str = "taco"
    """
    what does a want?
    """

    def execute(self):
        print(f'A says {self.a_config}')


@dataclass
class DoB(Serializable):
    b_config: str = "pie"
    """
    what does b want?
    """

    def execute(self):
        print(f'B says {self.b_config}')


@dataclass
class Subprogram(Serializable):

    command: DoA | DoB
    """
    Do A or B?
    """

    def execute(self):
        self.command.execute()

if __name__ == "__main__":

    parser = ArgumentParser(add_config_path_arg=True)

    parser.add_arguments(Subprogram, dest="sub")

    # logging.basicConfig(level=logging.DEBUG, 
    #                     format="%(filename)s:%(lineno)s:%(name)s:%(message)s")
    args = parser.parse_args()

    sub: Subprogram = args.sub

    with open('./mwe_config_out.json', 'w') as ofile:
        sub.dump_json(ofile)

    sub.execute()

Expected behavior
Running

python sp_mwe.py doa --config_path mwe_config.json

with sp_mew.py being the above source code and mwe_config.json containing either:

    "sub": {
        "command": {
            "doa": {
                "a_config": "taco cheese"
            },
            "dob": {
                "b_config": "apple pie"
            }
        }
    }
}

or

    "sub": {
        "command": {
                "a_config": "taco cheese",
                "b_config": "apple pie"
            }
        }
    }

(I don't care which format, either would be fine) should produce the output

A says taco cheese

Actual behavior
Instead, using either of the json formats we get the default output of

A says taco

Desktop (please complete the following information):

  • Version 0.1.0 (3122f6e)
  • Python version: 3.11.0
@aliounis
Copy link
Author

aliounis commented Apr 10, 2023

After some investigating, I can confirm that adding

            if isinstance(self.default, dict) and self.default.get(subcommand, None) is not None:
                subparser.add_arguments(dataclass_type, dest=self.dest, default=dataclass_type(**self.default.get(subcommand)))
            else:
                subparser.add_arguments(dataclass_type, dest=self.dest)

right after https://github.com/lebrice/SimpleParsing/blob/master/simple_parsing/wrappers/field_wrapper.py#L1033
produces the anticipated behavior when using the first config file in the original post (with doa and dob as entries). I am not sure if this would break other things, but at the very least it seems to work for my use case. If people think this is a good fix I can submit a PR

@lebrice
Copy link
Owner

lebrice commented Apr 13, 2023

Hello there @aliounis , thanks so much for posting this!

I'll try to look into this soon, but if you feel up to it and have time, sure, go ahead and create a PR!

This config_path argument seems to be causing issues (see #239 ), so hopefully we can kill two birds with one stone here.

Thanks again for posting!

aliounis added a commit to aliounis/SimpleParsing that referenced this issue Apr 17, 2023
…e not being passed when using subparsers (field_wrapper.py). Additionally fixes an issue where subdataclasses were requiring every value to be defaulted in the config path, instead of falling back to the default in the dataclass definition if it wasn't (dataclass_wrapper.py)
@aliounis
Copy link
Author

I submitted the above PR. This also grabs a second bug fix where subdataclasses had to be fully specified int he config file or not at all (partial specification was not allowed) instead of falling back to the standard default as it does for non-dataclass types. Let me know if you'd like me to open a second issue to track this other bug or if the PR/this comment is fine.

lebrice pushed a commit to aliounis/SimpleParsing that referenced this issue Jul 11, 2023
…e not being passed when using subparsers (field_wrapper.py). Additionally fixes an issue where subdataclasses were requiring every value to be defaulted in the config path, instead of falling back to the default in the dataclass definition if it wasn't (dataclass_wrapper.py)
@tcapelle
Copy link

tcapelle commented Jul 3, 2024

Is this going to be fixed? is there are workaround?

@lebrice
Copy link
Owner

lebrice commented Jul 3, 2024

Hello there @tcapelle, sorry no work has been done on this in a while. I will take a look when I find the time, but you're more than welcome to take a crack at it in the meantime! Let me know if you encounter any issues. #243 is an attempt at a fix, but broke a few tests. I'd start from there if I were you.

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 a pull request may close this issue.

3 participants