Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unhandled arguments checked after execution, not before #168

Open
lopuhin opened this issue Mar 19, 2019 · 10 comments
Open

Unhandled arguments checked after execution, not before #168

lopuhin opened this issue Mar 19, 2019 · 10 comments

Comments

@lopuhin
Copy link

@lopuhin lopuhin commented Mar 19, 2019

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
Copy link
Member

@dbieber dbieber commented Mar 19, 2019

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
Copy link
Author

@lopuhin lopuhin commented Mar 19, 2019

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
Copy link

@trhodeos trhodeos commented Mar 20, 2019

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
Copy link
Author

@lopuhin lopuhin commented Mar 20, 2019

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
Copy link

@gwern gwern commented Mar 20, 2019

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
Copy link
Member

@dbieber dbieber commented Mar 20, 2019

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
Copy link

@dreamflasher dreamflasher commented Jul 28, 2020

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

@mgielda
Copy link

@mgielda mgielda commented Oct 27, 2020

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
Copy link
Member

@dbieber dbieber commented Oct 30, 2020

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

while True:

There are two places where we dispatch function calls to user code:
component, remaining_args = _CallAndUpdateTrace(

and
component, remaining_args = _CallAndUpdateTrace(

_CallAndUpdateTrace uses parse to determine which arguments to use to call the user function, and which arguments will remain. parse is defined here
parse = _MakeParseFn(fn, metadata)

The user function is called here
component = fn(*varargs, **kwargs)

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
Copy link

@danieldugas danieldugas commented Oct 12, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants