classification
Title: argparse: positional with type=int, default=SUPPRESS raise ValueError
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: fhsxfhsx, n8falke, paul.j3
Priority: high Keywords:

Created on 2019-02-22 14:18 by n8falke, last changed 2020-01-18 01:30 by paul.j3.

Messages (7)
msg336314 - (view) Author: Axel (n8falke) Date: 2019-02-22 14:18
Example source:
from argparse import ArgumentParser, SUPPRESS
==============
parser = ArgumentParser()
parser.add_argument('i', nargs='?', type=int, default=SUPPRESS)
args = parser.parse_args([])
==============
results in:
error: argument integer: invalid int value: '==SUPPRESS=='

Expected: args = Namespace()


In Lib/argparse.py:
line 2399 in _get_value: result = type_func(arg_string)
with arg_string = SUPPRESS = '==SUPPRESS=='

called by ... line 1836 in take_action: argument_values = self._get_values(action, argument_strings)
which is done before checking for SUPPRESS in line 1851:
    if argument_values is not SUPPRESS:
       action(...)
msg336320 - (view) Author: Axel (n8falke) Date: 2019-02-22 15:03
Some more details:
The problem is not the order of assignment in take_action:
Defaults have been set by:
    def parse_known_args(self, args=None, namespace=None):
        ...
        # add any action defaults that aren't present
        for action in self._actions:
            if action.dest is not SUPPRESS:
                if not hasattr(namespace, action.dest):
                    if action.default is not SUPPRESS:
                        setattr(namespace, action.dest, action.default)

Assignment without argument should not happen, like the example shows:
==============
from argparse import ArgumentParser
parser = ArgumentParser()
parser.add_argument('i', action="count", default=42)
args = parser.parse_args([])
print(repr(args))
==============
Namespace(i=43)
==============
msg336340 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2019-02-22 17:57
Defaults are handled into two stages.

At the start of parsing defaults are added to the Namespace.

At the end of parsing intact defaults are evaluated with 'type'.

But a nargs='?' positional gets special handling.  It matches an empty string, so it is always 'seen'.  If its default is not None, that default is put in the Namespace instead of the matching empty list.

It's this special default handling that lets us use a ?-positional in a mutually exclusive group.

I suspect the error arises from this special default handling, but I'll have to look at the code to verify the details.
msg336347 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2019-02-22 21:52
By defining a custom 'type' function:

    def foo(astr):
        if astr is argparse.SUPPRESS:
            raise KeyError
        return astr

I get the full traceback

   1831         def take_action(action, argument_strings, option_string=None):
   1832             seen_actions.add(action)
-> 1833             argument_values = self._get_values(action, argument_strings)

and in '_get_values' the error is produced when it calls '_get_value' (which runs the 'type' function):

        # optional argument produces a default when not present
        if not arg_strings and action.nargs == OPTIONAL:
            if action.option_strings:
                value = action.const
            else:
                value = action.default
            if isinstance(value, str):
 -->            value = self._get_value(action, value)
                self._check_value(action, value)
  
It identifies this as an OPTIONAL action that has received an empty argument list, and assigns it the action.default.

ZERO_OR_MORE * also gets the action.default, but without a _get_value() call.  That default can be SUPPRESSed by the test at the end of take_action.

A couple of fixes come to mind: 

- add a SUPPRESS test at the start of take_action
- add a SUPPRESS test to _get_values block I quote above, maybe bypassing the `_get_value` call

There is a unittest case of a suppressed optional positional; it just doesn't also test for a failed type.

class TestDefaultSuppress(ParserTestCase):
    """Test actions with suppressed defaults"""

    argument_signatures = [
        Sig('foo', nargs='?', default=argparse.SUPPRESS)

I'm inclined go with the second choice, but the alternatives need to be throughly tested.

In the mean time, an 'int' type could be replaced with one that is SUPPRESS knowledgeable:

    def bar(astr):
        if astr is argparse.SUPPRESS:
            return astr
        else:
            return int(astr)

Note that this use of action.default is different from the normal default handling at the start of parse_known_args (and the end of _parse_known_args).  It's specifically for positionals that will always be 'seen' (because an empty argument strings list satisfies their nargs).
msg336524 - (view) Author: Axel (n8falke) Date: 2019-02-25 14:39
Thanks for so fast looking into this.

Good idea to use the workaround with a own conversion function. I'll use this for now.

To see what's happening, I tried a own Action with print in __call__ and a own conversion function with printing. I found following workflow:
1) direct assignment of unconverted default value (if not SUPPRESS, in parse_known_args)
2) conversion of argument string into given type
3) call to Action.__call__ which sets the converted value
4) only in edge cases: Convert default into given type and set in target

When there is no option there is only:
default  | arg, narg = '?' | --opt  | arg, narg = '*'
-----------------------------------------------------
None     | 1),     3)      | 1)     | 1), 2) with []
SUPPRESS |     2)!         |        | 
str      | 1), 2), 3)      | 1)     | 1), 2)
not str* | 1),     3)      | 1), 4) | 1), 2)

*can be int, float or other calss

It gets more complex the deeper I get into the source...

Yes, your second choice has probably less side effects.
msg359955 - (view) Author: yang (fhsxfhsx) * Date: 2020-01-14 08:14
I ran into the same issue and looked into the code, and found it more complicated than I thought. The more I went on, more issues occur. I wonder if I should open a new issue, but I will first comment here. If you feel like this should be a new issue, I will open one then. And I apologize in advance for possible vaguenesses in this comment because I modified it several times as I explored the code and found more issues. (also because of my poor English):)

It seems the issue happens only on positional arguments but not optional ones. Empty optional arguments will not call `take_action` and default values are handled and converted after consuming all arguments.

It also leads to inconsistancy between positional and optional arguments behaviour. Positional arguments always go through `take_action`, but optional arguments don't if an argument doesn't appear.

This inconsistancy causes another I think is strange behaviour,

    parser = ArgumentParser()
    parser.add_argument('i', action='count')
    parser.parse_args([])

got

    Namespace(i=1)

On the other hand, in `_get_values` function, `_check_value` is called to handle `choices=`, but there is no such guard for optional arguments, which means,

    parser = ArgumentParser()
    parser.add_argument('-i', nargs='?', type=int, default='2', choices=[1])
    parser.parse_args([])

doesn't raise an error.

Besides Paul's two instructive solutions, I think it better to make both sides behave the same. However, I found things seem not that simple.

First, ZERO_OR_MORE, no default value, positional arguments have `required=True` by default, but

    parser.add_argument('foo', nargs='*')
    parser.parse_args([])

got no problems. So it at least appears not required. (The document says `required` is only for optionals, so I guess it's just a implementation level but not a user level thing)

Second, the last case above gives

    Namespace(foo=[])

which seems logically incorrect or at least controversial, because the default is not set and you give no arguments, how does this list come? The document says nothing about the case (it's true it's a very corner one) and it also differs from the optional arguments case which gives

    Namespace(foo=None)

A walk around which doesn't change it is possible and I've written a patch fixing it.
And I'm not sure what we usually do if I propose to make them give the same result, is a PEP needed or I just raise a discussion about it? The change might break current code.
msg360224 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2020-01-18 01:30
This is a complicated issue that needs a lot of thought and testing before we make any changes.

While all Actions have the 'required' attribute, the programmer can only set it for optionals.  _get_positional_kwargs() will raise an error if the programmer tries to set it for a positional.  For a positional its value is determined by the nargs value.

The distinction between positionals and optionals occurs through out argparse, so we shouldn't put much effort (if any) into making their handling more uniform.  (The fundamental distinction between the two is whether the action.option_strings list is empty or not.)

A fundamental difference in parsing is that an optional's Action is called only if the flag is used.  A positional's Action is always called.   

_parse_known_args starts with a list of positionals

    positionals = self._get_positional_actions()

The nested consume_positionals function removes actions from this list.

Earlier versions raised an error at the end of parsing if this list was not empty.  In the current version that's been replace by the 'required_actions' test (which tests both positionals and optionals).  It was this change over that resulted in the bug/feature that subparsers (a positionals Action) are no longer required (by default).

Positionals with nargs='?' and '*' pose an extra challenge.  They are, in a sense, no longer 'required'.  But they will always be 'seen' because their nargs is satisfied by an empty list of values.  But that would overwrite any 'default' in the Namespace.  So there's the added step in _get_values of (re)inserting the action.default.  And it's the handling of that 'default' that gives rise to the current issue.

These two positionals can also be used in a mutually_exclusive_group.  To handle that 'take_action' has to maintain both the 'seen_actions' set and the  'seen_non_default_actions' set.
History
Date User Action Args
2020-01-18 01:30:50paul.j3setmessages: + msg360224
2020-01-14 08:14:25fhsxfhsxsetnosy: + fhsxfhsx
messages: + msg359955
2019-03-07 20:16:45paul.j3setpriority: normal -> high
2019-02-25 14:39:07n8falkesetmessages: + msg336524
2019-02-22 21:52:40paul.j3setmessages: + msg336347
2019-02-22 17:57:05paul.j3setmessages: + msg336340
2019-02-22 15:12:57n8falkesetnosy: + paul.j3
2019-02-22 15:03:54n8falkesetnosy: - paul.j3
messages: + msg336320
2019-02-22 14:46:20xtreaksetnosy: + paul.j3
2019-02-22 14:18:34n8falkecreate