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

Saving the chosen subgroup name somewhere on args #139

Closed
lebrice opened this issue May 11, 2022 · 0 comments · Fixed by #140
Closed

Saving the chosen subgroup name somewhere on args #139

lebrice opened this issue May 11, 2022 · 0 comments · Fixed by #140
Assignees

Comments

@lebrice
Copy link
Owner

lebrice commented May 11, 2022

Currently, when using subgroups, the value for the subgroup name is not saved anywhere on the args, since the subgroup overwrites the original argument in the second phase of parsing:

@dataclass
class Config:
    person: Person = subgroups({"bob": Bob, "alice": Alice}, default=Bob)

parser = ArgumentParser()
parser.add_arguments(Config, "config")

args = parser.parse_args()
print(args)

Recap of Subgroups:

(As a recap, here is what subgroups do:)

$ python test/test_issue_blab.py --help
usage: test_issue_blab.py [-h] [--person {bob,alice}] [--person.age int] [--person.cool bool]

optional arguments:
  -h, --help            show this help message and exit

Config ['config']:
  Configuration dataclass.

  --person {bob,alice}  (default: bob)

Bob ['config.person']:
  Person named Bob.

  --person.age int      (default: 32)
  --person.cool bool    (default: True)
$ python issue.py --person alice --help
usage: test_issue_blab.py [-h] [--person {bob,alice}] [--person.age int] [--person.popular bool]

optional arguments:
  -h, --help            show this help message and exit

Config ['config']:
  Configuration dataclass.

  --person {bob,alice}  (default: bob)

Alice ['config.person']:
  Person named Alice.

  --person.age int      (default: 13)
  --person.popular bool
                        (default: True)

The problem

$ python issue.py --person alice
Namespace(config=Config(person=Alice(age=13, popular=True)))

It would be useful to be able to extract the name of the chose subgroup. For instance, when using a subgroup for a choice between the HParams class of different models, we want to extract the choice of the model:

@dataclass
class Config:
     model: Model.HParams = subgroups({
         "simple": SimpleModel.HParams,
         "advanced": AdvancedModel.HParams,
         }, default=SimpleModel.HParams)
         

parser = ArgumentParser()
parser.add_arguments(Config, "config")
args = parser.parse_args()
config: Config = args.config
model_hparams: Model.HParams = config.model

# ehhh, what kind of Model was chosen, exactly?
# --> Currently no way of knowing, except by doing something sneaky
#       like this:
# Extract type of model from type of HParams:
model_type_str = type(model_hparams).__qualname__.rpartition(".")[0]
# Find matching type
model_type = [
    model_type for model_type in Model.__subclasses__()
    if model_type.__qualname__ == model_type_str
][0]

This however doesn't work, since different models might share the same HParams class! It's not a viable solution.

I'm thinking that having something like a subgroups dictionary in the args itself could be useful. Something like this:

$ python issue.py --person alice
Namespace(config=Config(person=Alice(age=13, popular=True)), subgroups={'config.person': 'alice'})
@lebrice lebrice self-assigned this May 11, 2022
lebrice added a commit that referenced this issue Dec 20, 2022
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
lebrice added a commit that referenced this issue Jan 5, 2023
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
lebrice added a commit that referenced this issue Jan 10, 2023
* Adding test with current WIP implementation

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Removed the previous subgroups implementation

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix test collection for vscode

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Making some progress (need to fix postprocessing)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Making more progress, almost there (ish)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Making more progress (need to fix conflicts)

The FieldWrapper is created for the subgroups arg.
I now create a new DataclassWrapper and graft add it as a child of the
DataclassWrapper that contains the subgroup field.

The current issue seems to be that the ConflictResolver is seeing both
the FieldWrapper and the DataclassWrapper (I Think) as pointing to the
same destination!

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Conflicts confusion is clearing up

The `conflict_resolver.resolve` method was actually doing two things:
- Resolving conflicts
- Flattening the list!

This was causing confusion, since `wrappers` was secretely growing,
which was causing the issue with duplicates from before.

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Things are parsing, but default is slightly wrong

This is getting to the point where things are parsing, but the default
value for a part of the subgroup is wrong. Need to check what exactly is
going on.

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* 'subgroups' is only added on namespace when needed

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Progress: Might have identified issue

I think I figured out the issue. If we generate a DataclassWrapper
for a subgroup, then it doesn't get a real `Field`, which might be
preventign the parsing from working properly?

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Refactor _set_instances_on_namespace

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Refactor, fix some failing basic tests

Certain things were breaking in basic tests.

TODOs left:
- Still getting some errors in the ALWAYS_MERGE case
- Still not parsing the right values for the other arguments

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix small bug in _consume_constructor_arguments

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Refactoring: Improve naming of Parser methods

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* [dirty] wip (switching machines)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix mistake with self._wrappers, few tests left

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Simplify `subgroups` fn, begin rewriting tests

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Simple subgroup tests are passing

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix issue with .setdefault and new subgroups

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix required arg for equivalent_argparse_code

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Remove a logger.Critical call

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Remove hacky code from previous iteration

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Remove the `_print_tree` function

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Make first block of logging in test a bit better

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Apply pre-commit hooks to field_wrapper.py

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Cleanup the resolve_subgroups code a tiny bit

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Make `Dataclass` an actual Protocol

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Make DataclassWrapper use Dataclass typevar

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Add test to check that subgroups are saved (#139)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Make `subgroups` sig reflect current limitations

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Add more tests (not passing)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Refactor FieldWrapper.default

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix required subgroups issue

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix error in test for subgroups with a conflict

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Minor esthetic change to test_subgroups.py

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix issues with py37 and parsing of Reused lists

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Very minor improvement to unrelated test

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* [optional] Make the "reuse" logic a bit better?

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Revert "[optional] Make the "reuse" logic a bit better?"

This reverts commit b8787c8.

* Fix typing of the `subgroups` function

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Removed unused _remove_help_action and test

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* "create_dataclasses"->"instantiate_dataclasses"

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Remove commented subgroup tests

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Add more tests for the `subgroups` function

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Use dict instead of Mapping

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Add test for help string future feature

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

* Fix small bug in test

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
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.

1 participant