classification
Title: argparse: 'resolve' conflict handler damages the actions of the parent parser
Type: behavior Stage:
Components: Documentation, Library (Lib), Tests Versions: Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: bethard, deckar01, docs@python, kcwu, paul.j3
Priority: normal Keywords: patch

Created on 2014-09-14 05:46 by paul.j3, last changed 2021-01-27 18:49 by kcwu.

Files
File name Uploaded Description Edit
sample3.py paul.j3, 2014-09-26 07:20
patch_1.diff paul.j3, 2014-10-01 22:56 review
Messages (8)
msg226862 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-09-14 05:46
When there's a conflict involving an argument that was added via 'parents', and the conflict handler is 'resolve', the action in the parent parser may be damaged, rendering that parent unsuitable for further use.

In this example, 2 parents have the same '--config' argument:
  
    parent1 = argparse.ArgumentParser(add_help=False) 
    parent1.add_argument('--config')

    parent2 = argparse.ArgumentParser(add_help=False)
    parent2.add_argument('--config')

    parser = argparse.ArgumentParser(parents=[parent1,parent2],
        conflict_handler='resolve')

The actions of the 3 parsers are (from the ._actions list):

               (id,          dest,    option_strings)
    parent1:  [(3077384012L, 'config', [])]    # empty option_strings

    parent2:  [(3076863628L, 'config', ['--config'])]

    parser:   [(3076864428L, 'help', ['-h', '--help']),
               (3076863628L, 'config', ['--config'])]  # same id

The 'config' Action from 'parent1' is first copied to 'parser' by reference (this is important).  When 'config' from 'parent2' is copied, there's a conflict.  '_handle_conflict_resolve()' attempts to remove the first Action, so it can add the second.  But in the process it ends up deleting the 'option_strings' values from the original action.

So now 'parent1' has an action in its 'optionals' argument group with an empty option_strings list.  It would display as an 'optionals' but parse as a 'positionals'.  'parent1' can no longer be safely used as a parent for another (sub)parser, nor used as a parser itself.

The same sort of thing would happen, if, as suggested in the documentation:

    "Sometimes (e.g. when using parents_) it may be useful to simply
     override any older arguments with the same option string." 

In test_argparse.py, 'resolve' is only tested once, with a simple case of two 'add_argument' statements.  The 'parents' class tests a couple of cases of conflicting actions (for positionals and optionals), but does nothing with the 'resolve' handler.

------------------------------

Possible fixes:

- change the documentation to warn against reusing such a parent parser

- test the 'resolve' conflict handler more thoroughly

- rewrite this conflict handler so it does not modify the action in the parent

- possibly change the 'parents' mechanism so it does a deep copy of actions.

References:

http://stackoverflow.com/questions/25818651/argparse-conflict-resolver-for-options-in-subcommands-turns-keyword-argument-int

http://bugs.python.org/issue15271 
argparse: repeatedly specifying the same argument ignores the previous ones

http://bugs.python.org/issue19462
Add remove_argument() method to argparse.ArgumentParser

http://bugs.python.org/issue15428
add "Name Collision" section to argparse docs
msg226982 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-09-17 07:03
'sample3.py' contains a '_handle_conflict_parent' function.

With the help of a change in '_add_container_actions' it determines whether a conflicting action occurs in multiple parsers (and argument_groups).  If it does, it deletes it from the correct group, without altering either the action, or other parsers.

If the action occurs in only 1 group, then the 'resolve' method is used.

The testing script mixes parents, subparsers, and various conflicts.
msg228137 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2014-10-01 22:56
A simpler solution is to make a copy of each Action when importing a parent parser.  The current practice is to just copy references.  With a copy, an Action will belong to only one group and parser, and the 'resolve' handler will operate without problems.

In the attached patch, I added a `.copy` method to Action.  I believe '.option_strings' is the only attribute that needs special handling, since it is a list, and 'resolve' operates on it.  The other attributes are strings, or objects that the user defines (e.g. 'default', 'choices').

The other change is in '_add_container_actions', the method which imports parents.  Here I make the copy action contingent on a 'action_copy' attribute of the conflict handler object (a function).  I also define this attribute for the 'resolve' handler.  I've made this copy contingent just to be safe w/r to backward compatibility, even though, I can't think of a reason for preferring the existing copy by reference.

In another Stackoverflow question, a poster wanted to use the same parent for 2 subparsers, but give the 2 actions different defaults.  This copy approach solves that issue.

This patch needs testing and documentation changes.
msg262314 - (view) Author: Jared Deckard (deckar01) Date: 2016-03-23 23:06
This behavior is preventing me from using more than one parent parser.

My use case is a convenience subcommand that performs two existing subcommands, therefore logically its subparser is required to support the arguments of both subparsers.

The only conflict is the "help" argument, which is annoying, but setting the conflict handler to "resolve" on the child subparser should just ignore the first parent's "help" argument in favor of the second parent.

Instead the "resolve" conflict handler breaks the first parent and causes it to output the help info no matter what arguments are given to it.

I am forced to only include one parent and manually duplicate the arguments for any additional parents.
msg262318 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2016-03-24 01:57
Is this the kind of scenario that you have problems with?

    parser = argparse.ArgumentParser()
    sp = parser.add_subparsers(dest='cmd')
    p1 = sp.add_parser('cmd1')
    p1.add_argument('-f')

    p2 = sp.add_parser('cmd2')
    p2.add_argument('-b')

    p3 = sp.add_parser('cmd3', parents=[p1,p2],add_help=False, conflict_handler='resolve')
    p3.add_argument('-c')

The problem is, apparently, that 'resolve' removes the '-h' option_string from the 1st subparser (but does not delete the whole action).  A kludgy fix is to add the '-h' back in (after p3 is created):

    p1._actions[0].option_strings=['-h']

'p1._actions' is the list of actions (arguments) for subparser p1.  Usually help is the first action (if isn't in p3 because of the parents resolve action).

I may work out a custom conflict handler function, but for now this appears to solve the issue.  I don't know if the patch I proposed solves your issue or not.
msg262365 - (view) Author: Jared Deckard (deckar01) Date: 2016-03-24 16:46
That is correct. (Thank you for whipping up a repro script from my description).

I appreciate the work around, but it is nearly as verbose as manually duplicating the parameters on the child and I would have to type up a large comment block to educate future developers on the hack.

Is there some documentation I am missing or is the "resolve" conflict handler broken?

> The parents= argument takes a list of ArgumentParser objects, collects all the positional and optional actions from them, and adds these actions to the ArgumentParser object being constructed.
https://docs.python.org/3.6/library/argparse.html#parents

It would be nice if the "error" conflict handler ignored conflicts when the actions are identical.

It should be expected that the "resolve" conflict handler would not modify the parents. It is exhibiting undocumented and undesirable side effect, which indicates to me that it is a bug not a missing feature.

As for the patch, I'm not sure why you would ever want "action_copy" to be False. It seems like creating copies to insulate the originals ignores the underlying issue that these parents are being mutated when the operation implies a union of properties with no side effects.

I don't think the fix for this should be behind an optional flag, I think the fix is to update "resolve" to take into consideration multiple parents like the custom conflict handler in your sample.
msg262366 - (view) Author: Jared Deckard (deckar01) Date: 2016-03-24 16:48
Adding back components and version data I unintentionally removed in http://bugs.python.org/msg262314
msg262373 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2016-03-24 20:16
Neither 'parents' or 'resolve' are documented in any detail.  Most of what I know comes from reading the code.

'parents' are, I think, most useful when importing a parser from some other module.  It lets you add arguments to your own parser without digging into the other module's code.  And 'resolve' lets you overwrite arguments.  

Copying arguments by reference is the easiest thing to do in Python, and apparently no one asked, 'what if the user wants to use the parent independently?'

Note I had to add a 'copy' method, because nothing else in 'argparse' needs it.  And no one has written code to check whether two Actions are the same (other than the obvious one of checking object ids).  Again, no need.

Copy and paste and utility functions are great ways of defining multiple arguments when mechanisms like 'parents' prove to be too clumsy.  Look at `test_argparse.py` for examples of utility code that streamlines parser definition.  Each test case requires a new parser (or more).
History
Date User Action Args
2021-01-27 18:49:22kcwusetnosy: + kcwu
2016-03-24 20:16:19paul.j3setmessages: + msg262373
2016-03-24 16:48:34deckar01setmessages: + msg262366
components: + Documentation, Tests
versions: + Python 3.5
2016-03-24 16:46:19deckar01setmessages: + msg262365
2016-03-24 01:57:08paul.j3setmessages: + msg262318
2016-03-23 23:06:50deckar01setversions: + Python 2.7, - Python 3.5
nosy: + deckar01

messages: + msg262314

components: - Documentation, Tests
2014-10-01 22:56:07paul.j3setfiles: + patch_1.diff
keywords: + patch
messages: + msg228137
2014-09-26 07:20:35paul.j3setfiles: - sample3.py
2014-09-26 07:20:19paul.j3setfiles: + sample3.py
2014-09-20 23:00:16paul.j3setnosy: + bethard
2014-09-19 06:08:27paul.j3setfiles: - sample3.py
2014-09-19 06:08:05paul.j3setfiles: + sample3.py
2014-09-18 07:26:14paul.j3setfiles: - sample3.py
2014-09-18 07:25:51paul.j3setfiles: + sample3.py
2014-09-17 07:03:16paul.j3setfiles: + sample3.py

messages: + msg226982
2014-09-14 05:46:38paul.j3create