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

argparse does not preserve namespace with subparser defaults #89398

Open
ALSchwalm mannequin opened this issue Sep 17, 2021 · 23 comments · May be fixed by #30219
Open

argparse does not preserve namespace with subparser defaults #89398

ALSchwalm mannequin opened this issue Sep 17, 2021 · 23 comments · May be fixed by #30219
Labels
3.12 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ALSchwalm
Copy link
Mannequin

ALSchwalm mannequin commented Sep 17, 2021

BPO 45235
Nosy @rhettinger, @ambv, @pablogsal, @miss-islington, @frostming, @RhinosF1, @ALSchwalm, @dgw, @lwalejko
PRs
  • bpo-45235: Fix argparse overrides namespace with subparser defaults #28420
  • [3.10] bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420) #28442
  • [3.9] bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420) #28443
  • bpo-45235: Revert an argparse bugfix that caused a regression #29525
  • [3.10] bpo-45235: Revert an argparse bugfix that caused a regression (GH-29525) #29530
  • [3.9] bpo-45235: Revert an argparse bugfix that caused a regression (GH-29525) #29531
  • bpo-45235: Fix argparse namespace overridden by subparsers default #29574
  • gh-89398: Fix argparse namespace overridden by subparsers default #30219
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2021-09-17.14:58:54.103>
    labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7', 'library']
    title = 'argparse does not preserve namespace with subparser defaults'
    updated_at = <Date 2021-12-22.19:20:37.779>
    user = 'https://github.com/ALSchwalm'

    bugs.python.org fields:

    activity = <Date 2021-12-22.19:20:37.779>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2021-09-17.14:58:54.103>
    creator = 'ALSchwalm'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45235
    keywords = ['patch']
    message_count = 23.0
    messages = ['402060', '402120', '402121', '402122', '404990', '404991', '405007', '405129', '406052', '406079', '406121', '406123', '406128', '406171', '406172', '406175', '406186', '406231', '406232', '406233', '406571', '406814', '408991']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'lukasz.langa', 'paul.j3', 'pablogsal', 'miss-islington', 'Frost Ming', 'RhinosF1', 'ALSchwalm', 'dgw', 'lwalejko']
    pr_nums = ['28420', '28442', '28443', '29525', '29530', '29531', '29574', '30219']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45235'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @ALSchwalm
    Copy link
    Mannequin Author

    ALSchwalm mannequin commented Sep 17, 2021

    The following snippet demonstrates the problem. If a subparser flag has a default set, argparse will override the existing value in the provided 'namespace' if the flag does not appear (e.g., if the default is used):

        import argparse
        parser = argparse.ArgumentParser()
        sub = parser.add_subparsers()
        example_subparser = sub.add_parser("example")
        example_subparser.add_argument("--flag", default=10)
        print(parser.parse_args(["example"], argparse.Namespace(flag=20)))

    This should return 'Namespace(flag=20)' because 'flag' already exists in the namespace, but instead it returns 'Namespace(flag=10)'. This intended behavior is described and demonstrated in the second example here: https://docs.python.org/3/library/argparse.html#default

    Lib's behavior is correct for the non-subparser cause.

    @ALSchwalm ALSchwalm mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Sep 17, 2021
    @rhettinger
    Copy link
    Contributor

    New changeset a6e8db5 by Adam Schwalm in branch 'main':
    bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420)
    a6e8db5

    @rhettinger
    Copy link
    Contributor

    New changeset 6e4101a by Miss Islington (bot) in branch '3.10':
    bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420) (GH-28442)
    6e4101a

    @rhettinger
    Copy link
    Contributor

    New changeset a18d522 by Miss Islington (bot) in branch '3.9':
    bpo-45235: Fix argparse overrides namespace with subparser defaults (GH-28420) (GH-28443)
    a18d522

    @rhettinger rhettinger self-assigned this Sep 18, 2021
    @rhettinger rhettinger self-assigned this Sep 18, 2021
    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Oct 25, 2021

    I haven't studied or tested this change, but it looks like a partial retraction of

    https://bugs.python.org/issue9351
    argparse set_defaults on subcommands should override top level set_defaults

    Originally the main namespace was passed to the subparser. Steven Bethard changed it so that the subparser got a fresh namespace, and all values were copied to the main namespace.

    I and others raised the question of how it affected user provided values

    https://bugs.python.org/issue27859
    argparse - subparsers does not retain namespace

    Looks like this patch tries to solve some problems by moving the self._defaults step to the end of parser_know_args. I view that change with some trepidation. Handling defaults is tricky enough, with setting them at the start, but then only passing them through 'type' at the end if they still match the original strings.

    Mostly I've been telling StackOverflow questioners that it best not to use the same argument 'dest' in both the main and subparsers. Flags can be the same, but the 'dest' should be different to avoid conflicts over which default has priority.

    Again, I haven't tested this change, but I have a gut feeling it could have backward compatibility issues.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Oct 25, 2021

    I just downloaded this argparse.py.

    This change makes it impossible to use a subparser argument if it is defined in the user provided namespace, or by the main parser. It blocks not only subparser default, but also user input.

    It has reverted the 9351 patch which dates to 2014.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Oct 25, 2021

    parser = argparse.ArgumentParser()
        sub = parser.add_subparsers()
        example_subparser = sub.add_parser("example")
        example_subparser.add_argument("--flag", default=10)
        print(parser.parse_args(["example","--flag=15"], argparse.Namespace(flag=20)))

    still returns flag=20

    User input should override values set by the provided Namespace.

    @paulj3 paulj3 mannequin reopened this Oct 26, 2021
    @paulj3 paulj3 mannequin reopened this Oct 26, 2021
    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Oct 27, 2021

    I should study previous posts in more detail, but here are some thoughts on correctly handling user namespace.

    At the start of parse_known_args, there's a

            if namespace is None:
                namespace = Namespace()

    We need to hang on to a copy of this namespace, e.g. call it

            import copy
            orig_namespace = copy.copy(namespace)

    In _SubParsersAction.__call__, pass this copy to the subparser (instead of None):

    subnamespace, arg_strings = parser.parse_known_args(arg_strings, orig_namespace)
        for key, value in vars(subnamespace).items():
            setattr(namespace, key, value)
    

    Prior to 9351, the main namespace was passed to the subparser

    namespace, arg_strings = parser.parse_known_args(arg_strings, namespace)
    

    The trick is to get orig_namespace from the main parse_known_args to SubParsersAction.__call__ method.

    in a 9351 post I explore the idea of allowing the user to specify a 'sub_namespace' for the subparser.

    https://bugs.python.org/msg230056

    In any case, this is a complicated issue that needs careful thought and more extensive testing. I didn't entirely like the original 9351 change, but since that's been part of argparse for many years, we need to very careful about clobbering it.

    @dgw
    Copy link
    Mannequin

    dgw mannequin commented Nov 9, 2021

    Can confirm that this patch DOES cause backward compatibility issues, as paul.j3's gut feeling said it could. One of my projects, testing against 3.6-3.9, now fails its test suite on Python 3.9.8, which includes this change. Arguments passed to a subparser are indeed ignored in lieu of default values. We are tracking the problem in our own issue, sopel-irc/sopel#2210

    I, for one, am not amused that 7-year-old behavior was "clobbered" (as previously described in this thread) in a patch release.

    @lwalejko
    Copy link
    Mannequin

    lwalejko mannequin commented Nov 10, 2021

    I can also confirm that there is an regression in Python 3.9.8 regarding argparse

    For example, using watchdog 2.1.6 package

    Python 3.9.7 (correct behaviour)
    In [1]: from watchdog import watchmedo
    In [2]: watchmedo.cli.parse_args(["auto-restart", "echo", "123"])
    Out[2]: Namespace(command='echo', command_args=['123'], directories=None, patterns='*', ignore_patterns='', ignore_directories=False, recursive=False, timeout=1.0, signal='SIGINT', debug_force_polling=False, kill_after=10.0, func=<function auto_restart at 0x7f296d1d9dc0>)

    Python 3.9.8 (incorrect behaviour)
    In [1]: from watchdog import watchmedo
    In [2]: watchmedo.cli.parse_args(["auto-restart", "echo", "123"])
    Out[2]: Namespace(command='auto-restart', command_args=['123'], directories=None, patterns='*', ignore_patterns='', ignore_directories=False, recursive=False, timeout=1.0, signal='SIGINT', debug_force_polling=False, kill_after=10.0, func=<function auto_restart at 0x7fc39480cee0>)

    @gvanrossum
    Copy link
    Member

    This has caused a regression, also reported here:
    Azure/azure-cli#20269 Global Arguments stop working in Python 3.9.8

    Can we please get some attention from @rhettinger and @ambv?

    @rhettinger
    Copy link
    Contributor

    Unless anyone objects, I'll revert this across all affected branches.

    @ambv
    Copy link
    Contributor

    ambv commented Nov 10, 2021

    Go for it, Raymond.

    @rhettinger
    Copy link
    Contributor

    How long until the next bugfix releases for 3.9 and 3.10? To avoid further snowballing, it would be great to have this reversion pushed out soonish.

    @pablogsal
    Copy link
    Member

    The next bugfixe release of 3.10 is the 6th of December

    @rhettinger
    Copy link
    Contributor

    Only 3.9 needs an expedited rerelease.

    @rhettinger
    Copy link
    Contributor

    New changeset 807f839 by Raymond Hettinger in branch 'main':
    bpo-45235: Revert an argparse bugfix that caused a regression (GH-29525)
    807f839

    @rhettinger
    Copy link
    Contributor

    New changeset 587ff7f by Miss Islington (bot) in branch '3.9':
    bpo-45235: Revert an argparse bugfix that caused a regression (GH-29525) (GH-29531)
    587ff7f

    @rhettinger
    Copy link
    Contributor

    New changeset e4c5a5e by Miss Islington (bot) in branch '3.10':
    bpo-45235: Revert an argparse bugfix that caused a regression (GH-29525) (GH-29530)
    e4c5a5e

    @rhettinger
    Copy link
    Contributor

    I've restored the prior state of affairs.

    Leaving this issue open because it still isn't clear what should be guaranteed or whether further improvements need to be made.

    @rhettinger
    Copy link
    Contributor

    Paul, should this be closed or do you think there is still a namespace issue to be resolved?

    @FrostMing
    Copy link
    Mannequin

    FrostMing mannequin commented Nov 23, 2021

    Hi, I noticed this bug because of the regression of Python 3.9.8. And I proposed a better approach in PR 29574.

    Maybe the folks here can have a look. Thanks

    • Frost

    @FrostMing
    Copy link
    Mannequin

    FrostMing mannequin commented Dec 21, 2021

    Per the review comments of @jiasli, I worked out a second PR to fix this issue.

    This fix has less side-effect and better backward compatibility. I will leave the two PRs open. Any feedback is welcome.

    • Frost

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    trini pushed a commit to trini/u-boot that referenced this issue Jul 8, 2022
    On python 3.8.10 (and 3.10), subparsers are not updated with defaults. I
    suspect this is related to [1]. Fix this by explicitly updating
    subparsers with settings.
    
    [1] python/cpython#89398
    
    Fixes: 3145b63 ("patman: Update defaults in subparsers")
    Signed-off-by: Sean Anderson <sean.anderson@seco.com>
    Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
    Tested-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
    @terryjreedy terryjreedy added 3.12 bugs and security fixes and removed 3.11 only security fixes 3.10 only security fixes 3.9 only security fixes 3.8 only security fixes 3.7 (EOL) end of life labels Jan 7, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.12 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: Bugs
    Status: No status
    Development

    Successfully merging a pull request may close this issue.

    5 participants