bpo-45235: Fix argparse namespace overridden by subparsers default #29574

frostming · 2021-11-16T08:29:53Z

This PR is another attempt to fix bpo-45235.
In this patch, setting the default values is postponed until parsing is done and if the value is absent in the namespace. Also added a test case to cover the regression issue.

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

https://bugs.python.org/issue45235

the-knights-who-say-ni · 2021-11-16T08:29:56Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@frostming

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

jiasli · 2021-12-13T12:08:42Z

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

This is not true 🙁. We (Azure CLI) and IPython (commented by @tacaswell) both rely on the the default value to be there during an Action:

Global Arguments stop working in Python 3.9.8 Azure/azure-cli#20269 (comment)
bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment)

This looks like a BREAKING CHANGE to me. @rhettinger @ambv

frostming · 2021-12-13T14:05:39Z

As a side-effect, the default values of other arguments won't be seen during parsing(such as inside an action). But IMO it is an invalid use case since the value may be later overridden by a passed-in value.

This is not true 🙁. We (Azure CLI) and IPython (commented by @tacaswell) both rely on the the default value to be there during an Action:

Global Arguments stop working in Python 3.9.8 Azure/azure-cli#20269 (comment)

[bpo-45235: Fix argparse overrides namespace with subparser defaults bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment)](bpo-45235: Fix argparse overrides namespace with subparser defaults #28420 (comment))

This looks like a BREAKING CHANGE to me. @rhettinger @ambv

I meant relying on the default values before parsing is finished can be fragile since it depends on the parsing order of the arguments. Think of this example:

command [--foo=VALUE] [--callback]

Here --foo is an option with the default value of 5. And --callback is bound to an action that reads the value of foo on the Namespace object. If the default values are assigned before the parsing, it will see different values of foo:

command --foo=10 --callback   # callback sees foo=10
command --callback --foo=10   # callback sees foo=5

It is true that with the patch of this PR the callback action can by no means know the value of foo unless it is given before --callback. The default value only applies when all parsing is done and no value is given. This looks more explicit to me in the way that if you want to refer to the value of another option, you must give it before:

command --foo=10 --callback   # callback sees foo=10
command --callback --foo=10   # Error!
command --callback   # Error!

@jiasli Does it make more sense?

jiasli · 2021-12-14T02:30:06Z

Lib/argparse.py

+        # add any parser defaults that aren't present
+        for dest in self._defaults:
+            if not hasattr(namespace, dest):
+                setattr(namespace, dest, self._defaults[dest])


The default value we are referring to in an Action doesn't necessarily come from an argument like --foo. It may come from set_defaults:

command_parser.set_defaults( func=metadata, command=command_name, _cmd=metadata,

Thus, when calling self._parse_known_args, self._defaults will be in namespace.

If these 3 lines are moved down here, self._defaults won't be in namespace when calling self._parse_known_args, resulting in failure

File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 2003, in consume_optional take_action(action, args, option_string) File "C:\Users\name\AppData\Local\Programs\Python\Python310\lib\argparse.py", line 1931, in take_action action(self, namespace, argument_values, option_string) File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\commands\arm.py", line 357, in __call__ profile = Profile(cli_ctx=namespace._cmd.cli_ctx) # pylint: disable=protected-access AttributeError: 'Namespace' object has no attribute '_cmd'

I am open to whether this is the correct behavior, but it is definitely a BREAKING CHANGE given the behavior has been there for years. If this change is what Python committee want, it should be released in at least Python 3.12, not Python 3.10 or 3.11 which are already stabilized, per an email discussion with @gvanrossum:

Even in 3.11 such a breaking change would not be acceptable. They will have to figure out another way to provide the functionality.

This case makes sense to me. I admit that any change may be a breaking change to some people. This needs more discussion, it perhaps should happen on BPO I guess?

Yes, these are great concerns to get feedback from BPO-45235. Some backward compatibility concerns are already in that thread, and as maintainer of another project that was affected by the reverted change in Python 3.9.8, I'd love to see those get hashed out in the main thread instead of the line notes of this patch.

Could you in the meantime submit patches for the unit tests we’re missing?

There are at least two - for the regression that happened and the one that was identified here.

they are included in the one added case, do you suggest to separate into two cases?

I'm going to step away from this one. The last attempt at a fix was catastrophic and should teach us that almost every nuance of the implementation is being relied upon. I think you're correct in saying "any change may be a breaking change to some people. " ISTM that there is no possible benefit that would be worth breaking some long deployed, stable, working command-line tools

In my second attempt #30219 the side effect is removed, it should be cleaner.

The issue itself is a valid one, but the first fix isn't that careful.

MaxwellDupre · 2022-05-10T13:45:23Z

I think it would help if you added some clarifying info to the Docs under subparsers, possibly with examples. I think it also helps to explain internal workings which guides users to correct usage, but that's entirely up to you. I dont think you need to generate too much more stuff, just add whats in the bug report and documentation that you added to the code.
I note the subparsers Docs has not changed since 3.7.

bpo-45235: Fix argparse namespace overriden by subparsers default

c54e604

the-knights-who-say-ni added the CLA not signed label Nov 16, 2021

bedevere-bot added the awaiting review label Nov 16, 2021

📜🤖 Added by blurb_it.

71c1e0e

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 17, 2021

dgw mentioned this pull request Nov 25, 2021

argparse subparser defaults in Python 3.9+ sopel-irc/sopel#2210

Open

jiasli reviewed Dec 14, 2021

View changes

frostming mentioned this pull request Dec 21, 2021

gh-89398: Fix argparse namespace overridden by subparsers default #30219

Open

AbhigyanBose mentioned this pull request May 14, 2022

missing unittests for overlapping dest when using subparsers #89794

Open

ALSchwalm mannequin mentioned this pull request Apr 10, 2022

argparse does not preserve namespace with subparser defaults #89398

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

frostming commented Nov 16, 2021 •

edited

the-knights-who-say-ni commented Nov 16, 2021

jiasli commented Dec 13, 2021

frostming commented Dec 13, 2021 •

edited

jiasli Dec 14, 2021 •

edited

frostming Dec 14, 2021

dgw Dec 14, 2021

iritkatriel Dec 14, 2021 •

edited

frostming Dec 18, 2021

rhettinger Apr 16, 2022

frostming Apr 17, 2022

MaxwellDupre commented May 10, 2022

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

Are you sure you want to change the base?

bpo-45235: Fix argparse namespace overridden by subparsers default #29574

Conversation

frostming commented Nov 16, 2021 • edited

the-knights-who-say-ni commented Nov 16, 2021

CLA Missing

jiasli commented Dec 13, 2021

frostming commented Dec 13, 2021 • edited

jiasli Dec 14, 2021 • edited

Choose a reason for hiding this comment

frostming Dec 14, 2021

Choose a reason for hiding this comment

dgw Dec 14, 2021

Choose a reason for hiding this comment

iritkatriel Dec 14, 2021 • edited

Choose a reason for hiding this comment

frostming Dec 18, 2021

Choose a reason for hiding this comment

rhettinger Apr 16, 2022

Choose a reason for hiding this comment

frostming Apr 17, 2022

Choose a reason for hiding this comment

MaxwellDupre commented May 10, 2022

frostming commented Nov 16, 2021 •

edited

frostming commented Dec 13, 2021 •

edited

jiasli Dec 14, 2021 •

edited

iritkatriel Dec 14, 2021 •

edited