Repository navigation
argparse exits on error even when exit_on_error=False #103498
Description
Activity
Hi, new contributor here 👋
I written a quick fix which should be generic (instead of guarding all calls to
self.error, I added the guard in the method itself). But this way, throwing custom exceptions is not possible. (for example in the PR implementing the feature there was a test expecting aargparse.ArgumentErrorexception). Is this acceptable or is throwing custom exceptions desired?If it is desired, we could do the following without breaking the existing API:
def error(self, exception_or_message: Union[str, Exception])- allow passing both messages and exceptionsdef error(self, message: str, exception_class=None)- allow passing exception class information in a separate argument
Personally, I find the first one the simplest and requires little changes to the code. I would be glad if any maintainer could give an opinion.
As far as I can tell, all the
self.error()usage is reporting an issue with argument parsing(well, its theargparselibrary of course). In_parse_known_argsthere are mixed usage ofArgumentErrorandself.error()- which is pretty bad.Doing all the error handling in
self.error()seems reasonable, but I don't think we need to change the signature inself.error(). Just raiseArgumentErrorwith the message ifself.exit_on_errorisFalse- it's easier for user to catch it anyway.We also need to replace all the
raise ArgumentErrorwithself.error()to keep the code clean.Hmm, I did not realize that
ArgumentErroractually takes an action, that was my fault. I still think the current implementation is better, but there might be a backward compatibility issue involved. It's clearly stated that the user can overrideerror()function. If you added an extra argument and used it, the user's current code might break. I think we need some opinions from the component owner.Reacted by Mihail Feraru and Stephen Karl LarroqueNo, I did not reply to the email directly, I just wanted to edit the comment because I noticed the compatibility issue after I written it, but I deleted it instead. 😅
Anyway, I followed your advice, but stumbled upon this
actionparameter that breaks the API for classes that inheritArgumentParser. We could avoid this by using**kwargs, but it is not the cleanest way to solve it.Taking at the look at the code, when this feature was intruduced, it was not very thoroughly tested/reviewed. Basically all the
self.error()usage need to be vetted - by nature it contradicts this feature. So this feature either has to live inself.error(), or all the currentself.error()usage need to be protected by checking this option.- added a commit that references this issue
on Apr 14, 2023 For triage's sake, I think this is a duplicate of #85427, which has a PR waiting for review at #30832.
But this way, throwing custom exceptions is not possible. (for example in the PR implementing the feature there was a test expecting a argparse.ArgumentError exception). Is this acceptable or is throwing custom exceptions desired?
There's an even older duplicate with some discussion on this point that shaped the choices I made in #30832, see #30832 (comment).
Reacted by psasselum2 remaining items
I also came across the same issue where creating an
ArgumentParserwithexit_on_error=Falseand passing the wrong type of argument would raise anargparse.ArgumentError, but passing an unknown argument would exit the task.import argparse parser = argparse.ArgumentParser(exit_on_error=False) parser.add_argument("--integers", type=int) parser.parse_args(["--integers", "a"]). > argparse.ArgumentError: argument --integers: invalid int value: 'a' parser.parse_args(["--unknown", "a"]) > usage: olga.py [-h] [--integers INTEGERS] > olga.py: error: unrecognized arguments: --unknown a
Yes, as currently implemented
exit_on_erroronly controls errors that raise aargparse.ArgumentError. Theunrecognized argumentsexit does not useArgumentError(or an errorExceptionclass) , and so cannot be caught in the same way.A more comprehensive way of changing error handling is to customize either the
exitor `error methods:https://docs.python.org/3/library/argparse.html#exiting-methods
I think we are basically at a dead loop here - we want to keep the backward compatibility so we don't breaking anyone's existing code, but the current behavior is simply wrong. It's impossible to "fix" the behavior without breaking existing code.
The introduction of
exit_on_errordoes not live well with all thesys.exit()usage inargparse. There could be a lot of similar traps unless we patch all thesys.exit()s (self.error()).Anyone who had to make a decision about fixing this would carry plenty of pressure for breaking people's code and I believe according to @sobolevn none of the core devs are the experts of
argparsenow. I'm curious if/how we could proceed on this.Reacted by Olga Matoula and Mihail FeraruI took a deeper dive into the problem and I may have a solution. I think the main goals for the fix are:
- raise exceptions for all kinds of errors
ArgumentParsercould encounter - prevent future bugs related to
exit_on_error=Falseby calling.exit()indirectly by accident - stay as backward compatible as possible
First, to make the error handling consistent, I propose that we convert all calls to
self.error()to raising exceptions (in caseArgumentErrordoes not fit properly in some cases, we can think of a new public exception type, but I think we won't need that).Second, we make all public methods of
ArgumentParser(there are few) to handleexit_on_errorin an obvious way. Ifexit_on_error=False, we allow exception to pass through, otherwise we pass their string representation toself.errorwhich will do its thing and finally exit.I think that if we use
self.erroronly in simpletry-catchblocks in the public methods, it would be easy enough to make the right behaviour happen and avoid future mistakes.I will update the pull request to reflect what I was talking about and I'm looking for suggestions/opinions. Maybe we succeed to move this forward.
Below is a list of places where
self.exit/self.errorare used.Calls to ArgumentParser.exit which are justified and probably do not need to be changed:
_HelpAction.__call__ _VersionAction.__call__Calls to ArgumentParser.error that should be converted to raising exceptions:
ArgumentParser.add_subparsers ArgumentParser.parse_args ArgumentParser.parse_known_args ArgumentParser.parse_intermixed_args ArgumentParser._parse_known_args ArgumentParser._read_args_from_files ArgumentParser._parse_optional ArgumentParser._get_option_tuples- raise exceptions for all kinds of errors
I took a deeper dive into the problem and I may have a solution. I think the main goals for the fix are:
- raise exceptions for all kinds of errors
ArgumentParsercould encounter - prevent future bugs related to
exit_on_error=Falseby calling.exit()indirectly by accident - stay as backward compatible as possible
I'm sure there are ways to make the code better, but I believe the major issue here is not how, it is backward compatibility. For example, what if the user are subclassing
ArgumentParserand usingself.error()in their code? Your change would break the behavior.Like I said above, the real issue here is the library was used too much by the users and all the implementation details are considered "documented feature". We either keep the old "wrong" behavior, or break users existing code.
The hard part is to make the decision - and that's where we are stuck I believe. We need core devs to support a reform, even if that means breaking some of the user code. Without that, I don't believe there would be an implemetation feasible.
- raise exceptions for all kinds of errors
Hi,
The
exit_on_error=Falseis also used to ignore undefined arguments and return args collection of what argparse succeeded to parse. It should not raise exception as then the parse will not be able to return its value.Regards,
- `parse_known_args` can be used to return the unknowns along with the parsed ones. This alternative was available long before this `exit_on_error` parameter was introduced. The original, default behavior was `exit_on_error=True`. I doubt if there is much code that depends on the current incomplete implementation of the `False` alternative. The question is whether we can make the `False` coverage more complete without too much work, and without compromising the `True` case.…On Sun, Nov 5, 2023, 12:50 PM Alon Bar-Lev ***@***.***> wrote: Hi, The exit_on_error=False is also used to ignore undefined arguments and return args collection of what argparse succeeded to parse. It should not raise exception as then the parse will not be able to return its value. Regards, — Reply to this email directly, view it on GitHub <#103498 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAITB6ADV42JLQ32ZLWITZDYC73YVAVCNFSM6AAAAAAW4SRXYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTHA2DCOJTHA> . You are receiving this because you commented.Message ID: ***@***.***>
- I think before someone proposes a push, we need a comprehensive list of errors that still exit. ``exit_on_error=False` currently redirects the `ArgumentError` cases, which are tied to a specific argument. By default that error exits with a `usage` and argument specific message. `unknown arguments` exit is produced by `parse_args`, and isn't tied to any one argument. `parse_known_args` has, and still is available for bypassing that. I'm not sure that needs any further change. Mutually_exclusive_groups can produce errors; I don't know if `exit_on_error` addresses those There's also a test for `required_arguments`. That can affect multiple arguments, so it isn't an `ArgumentError`. I'm working here from memory, so there may be others.…On Sun, Nov 5, 2023 at 2:07 PM paulj ***@***.***> wrote: `parse_known_args` can be used to return the unknowns along with the parsed ones. This alternative was available long before this `exit_on_error` parameter was introduced. The original, default behavior was `exit_on_error=True`. I doubt if there is much code that depends on the current incomplete implementation of the `False` alternative. The question is whether we can make the `False` coverage more complete without too much work, and without compromising the `True` case. On Sun, Nov 5, 2023, 12:50 PM Alon Bar-Lev ***@***.***> wrote: > Hi, > > The exit_on_error=False is also used to ignore undefined arguments and > return args collection of what argparse succeeded to parse. It should not > raise exception as then the parse will not be able to return its value. > > Regards, > > — > Reply to this email directly, view it on GitHub > <#103498 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAITB6ADV42JLQ32ZLWITZDYC73YVAVCNFSM6AAAAAAW4SRXYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTHA2DCOJTHA> > . > You are receiving this because you commented.Message ID: > ***@***.***> >
Hi all,
the issue still exists for me in version 1.4.0.
Error stack trace:
...argparse.py:1829: in parse_args self.error(msg % ' '.join(argv)) argparse.py:2587: in error self.exit(2, _('%(prog)s: error: %(message)s\n') % args) argparse.py:2574: in exit _sys.exit(status)As I can see, on line 1857 it actually tries to verify 'if self.exit_on_error' and then use try..catch but in fact it doesn't reach that block as at line 1827 it goes to self.error that later leads to exit
- The preceding line is msg = _('unrecognized arguments: %s') This is the unrecognized arguments error that has been discussed in this thread. Contrary to what the docs imply, the 'don;t exit' parameter does not catch every kind of error. At the moment it just catches `ArgumentError` ones that are tied to one specific Argument. Broadening its coverage has been discussed, but, as far as I know, not been implemented.…On Mon, Dec 11, 2023 at 9:50 AM Andrii ***@***.***> wrote: Hi all, the issue still exists for me in version 1.3.0. Error stack trace: ... argparse.py:1829: in parse_args self.error(msg % ' '.join(argv)) argparse.py:2587: in error self.exit(2, _('%(prog)s: error: %(message)s\n') % args) argparse.py:2574: in exit _sys.exit(status) As I can see, on line 1857 it actually tries to verify 'if self.exit_on_error' and then use try..catch but in fact it doesn't reach that block as at line 1827 it goes to self.error that later leads to exit — Reply to this email directly, view it on GitHub <#103498 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAITB6FL6VKD6IJCNZWKVP3YI5BVHAVCNFSM6AAAAAAW4SRXYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJQGU3TSMZQG4> . You are receiving this because you commented.Message ID: ***@***.***>
@ericvsmith looking into this!
I wrote #121056 before discovering this issue. It fixes the issue in the way opposite to #103519: internal
error()calls are replaced with raisingArgumentError. It is then caught and passed toerror()at the higher level. Basically, it implements the idea proposed in #103498 (comment).We now have 3 duplicate issues with 4 open PRs (and one incomplete PR was already merged). It is time to finish this.
Reacted by Stephen Karl Larroque
Bug report
When
ArgumentParserencounters certain errors, it callsself.error(...)which exits, even whenexit_on_erroris set to False.This prevents catching the exception and writing a custom error message.
Test on Python 3.11.3:
Linked PRs