Unhandled arguments checked after execution, not before

[lopuhin]

Consider a simple program:

import fire

def add(a, zero=0):
    print('calculating...')
    print(a + zero)

if __name__ == '__main__':
    fire.Fire(add)

And then suppose we make a typo in the argument name, writing --zerro instead of --zero. This is what I get with fire 0.1.3 under Python 3.6:

$ python t.py 1 --zerro 2
calculating...
1
Fire trace:
1. Initial component
2. Called routine "add" (t.py:3)
3. ('Could not consume arg:', '--zerro')

Type:        NoneType
String form: None

Usage:       t.py 1 -

Notice that first we run the code, and only then the error is reported. While I expected the errors to be checked before any user code is executed, because this code could be working for a long time, doing wrong things, etc.

[dbieber]

Sorry to hear you're hitting this issue.

Unfortunately, there's not an obvious fix: Fire supports chaining functions, which means that the output of a function like add may determine what flags are valid for future functions. E.g. if add had returned a function which had an argument "zerro", then your command would have been valid. There's currently no way for Fire to know ahead of time that zerro wasn't going to be a valid argument for a subsequent function call.

Brainstorming possible workarounds:

1. If you remove the default argument for zero, then zero becomes a required flag. Fire won't execute the add function unless a value for zero is provided. This of course has the drawback that zero becomes a required flag, which isn't necessarily what you want.
2. You can add a decorator that lets you specify that a function should consume all arguments. Then you could decorate the "add" function with this decorator, and that would signal to Fire not to run the function unless all arguments are consumed as arguments to that function. I worked with someone recently who wrote such a decorator -- I'll ping him now and see if he's able to share it for you to use.

[lopuhin]

Thanks for a quick response @dbieber , I didn't realize that the chaining feature has these consequences, good to know that. Such a decorator would solve this issue indeed, thank you 👍

[trhodeos]

Hey @lopuhin, I wrote the decorator. I've pulled it into a gist here: https://gist.github.com/trhodeos/5a20b438480c880f7e15f08987bd9c0f. It should be compatible with python 2 and 3. Hope this helps!

[lopuhin]

This words great, thank you @trhodeos and @dbieber ! I only made a slight adjustment to the decorator to support keyword-only arguments (although this won't work on python 2 any more):

        argspec = inspect.getfullargspec(function_to_decorate)
        valid_names = set(argspec.args + argspec.kwonlyargs)

[gwern]

It's worth noting that this issue has hit 3 users of https://github.com/openai/gpt-2/ and those are just the ones I personally know of. EDIT: 4th user.

[dbieber]

Thanks for the feedback.

We may be able to fix this after all. The fix would be to require explicit chaining (using a separator, which is "-" by default) when not all the arguments are received, and only allow implicit chaining when all arguments have values. This would break some commands that are possible today, so we'll need to consider carefully if this change would be worthwhile.

[dreamflasher]

@dbieber Is there any way to explicitly turn off chaining? This should solve this problem, too, right?

[mgielda]

Hi everyone, how can we help push this forward? The inability to check whether the arguments provided are correct is definitely a large drawback to what otherwise is an awesome framework. I would say I'd prefer explicit chaining personally. The obvious default behavior would be for a CLI to fail if the signature is wrong.

[dbieber]

Thanks for the interest.

The change we're considering is to require explicit chaining (using a separator, which is "-" by default) when not all the arguments are received, and to only allow implicit chaining when all arguments have values. No one is actively working on this.

One implications of this change would be that functions that accept *args or **kwargs would always require explicit chaining.

If you want to help, some things you could do are:

• Try to determine if there are any reasonable commands this change would break backwards compatibility with
• Prototype the change - implementation and/or tests

How would the implementation work? Roughly, it would be something like this:

In the main while loop https://github.com/google/python-fire/blob/3be260e65a0c25d1dbbe1b15eeb0bf13ac7ec38f/fire/core.py#L425 There are two places where we dispatch function calls to user code: https://github.com/google/python-fire/blob/3be260e65a0c25d1dbbe1b15eeb0bf13ac7ec38f/fire/core.py#L463 and https://github.com/google/python-fire/blob/3be260e65a0c25d1dbbe1b15eeb0bf13ac7ec38f/fire/core.py#L553 _CallAndUpdateTrace uses parse to determine which arguments to use to call the user function, and which arguments will remain. parse is defined here https://github.com/google/python-fire/blob/3be260e65a0c25d1dbbe1b15eeb0bf13ac7ec38f/fire/core.py#L670 The user function is called here https://github.com/google/python-fire/blob/3be260e65a0c25d1dbbe1b15eeb0bf13ac7ec38f/fire/core.py#L672

It's at this point (before the call of fn) that we'd want to insert the new logic for checking if it's appropriate to call the function. In pseudocode, the logic would look like:

if fn has optional args that don't have values specified in varargs and kwargs and remaining_args is not empty:
  raise FireError('An error message here saying how the user probably specified the args wrong, or maybe they just want chaining, and if they want chaining they should use a separator explicitly') 

[danieldugas]

In case this interests some of you, here's a fire fork which is strict by default, meant as a temporary fix: https://github.com/danieldugas/python-strict-fire

[hponde]

Just checking if there is any interest in addressing this. I understand the chaining concern, but I feel like the ability to just pass in a strict argument to fire.Fire would be backwards compatible and address the issues in this thread, right?

Honestly most of my usage of fire is as follows and I can see it is the case for most other folks in the internet too.

if __name__ == "__main__":
    import fire
    fire.Fire(main)

to make CLIs easy to work with.

Which would then become:

if __name__ == "__main__":
    import fire
    fire.Fire(main, strict=True)

The fork here https://github.com/danieldugas/python-strict-fire from @danieldugas shows that the change is indeed not that large

[robotrapta]

For anybody else hitting this and frustrated that fire is so slow to respond to basic functionality that makes it extremely error-prone, I might suggest switching to typer: https://github.com/tiangolo/typer

For the common use-case it's a drop in replacement:

if __name__ == "__main__":
    import typer
    typer.run(main)

Plus it uses type-hints to you don't have to cast everything from a string.

[sweetcocoa]

For those of you who are looking for this feature but who cannot use typer, I found this is a simple workaround (using kwargs).

def main(wanted:str, **kwargs):
    if len(kwargs) > 0:
        print("Unknown options: ", kwargs)
        return
    print("wanted : ", wanted)

if __name__ == "__main__":
    fire.Fire(main)

[fmenasri-psee]

Just checking if there is any interest in addressing this. I understand the chaining concern, but I feel like the ability to just pass in a strict argument to fire.Fire would be backwards compatible and address the issues in this thread, right?

Honestly most of my usage of fire is as follows and I can see it is the case for most other folks in the internet too.

if __name__ == "__main__":
    import fire
    fire.Fire(main)

to make CLIs easy to work with.

Which would then become:

if __name__ == "__main__":
    import fire
    fire.Fire(main, strict=True)

The fork here https://github.com/danieldugas/python-strict-fire from @danieldugas shows that the change is indeed not that large

This fire.Fire(main, strict=True) proposed by @hponde seems like a very good compromise. @dbieber would that be ok for you ? Would a PR making those changes be accepted, or do you see any reasons to reject this proposition ?

[LRudL]

Chaining seems like a very niche feature, error-checking the arguments that the user types in seems like a top-3 feature for any CLI interface library. This issue is a dealbreaker for using Fire.

@hponde's proposal seems great.

[titu1994]

It would be valuable to support fire.Fire(main, strict=True). The alternative of typer has two major issues, one is it doesn't parse all the functions in the file automatically like using Fire() does, and second is that it has unique parsing of bool dtype - it doesn't parse True or False but generates the equivalent if --option and --no-option.

It is much preferred to parse bool-like input rather than force users to use argparse's --option and --no-option pattern.

Since the strict flag is entirely optional, and users would explicitly need to enable it to prevent chaining, you could invert the logic to say Fire(..., chain=True) as default and set chain=False to have strict arg parsing.

For now, the fork will suffice but it's not an ideal solution