Support SkipParse decorator

[link89]

Implement #403

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[dbieber]

By 'skip parse' is it that all arguments are treated as strings?
We can probably choose a clearer name for the decorator in that case.
I'll come back and give this more thought after the 0.5.0 release.

Update: (A: The context is in #403. Note to self: reference that while reading the PR.)

[link89]

By 'skip parse' is it that all arguments are treated as strings?

By SkipParse I just want Fire to pass remaining arguments to function by keeping their original form and order as they in the sys.argv, whose type is Tuple[str]. This feature is useful when using Fire to delegate argv to other command line tool, or user want to handle some special form of options by themselves. Another name I am considering is PassThrough .

[link89]

Hi @dbieber Do you have a chance to review this PR? I have some projects depend on this feature and it would be easier to build and delivery them if a new release of Fire can include this patch.

[dbieber]

Thanks for following up. I like the direction this PR was going. I think what we'll want here is a little more general than what you've implemented so far.

We'll want a decorator that causes Fire to still parse args for named and positional args and kwargs, but to pass any remaining args as *varargs unparsed.

This will behave the same for the example in your test case (which only has *varargs, no additional args), but would behave differently if there were other args to the function.

Brainstorming as to what to call this:

• @fire.decorators.Passthrough
• @fire.decorators.ArgvPassthrough
• @fire.decorators.ArgsPassthrough
• @fire.decorators.VarargsPassthrough

Wdyt? Would you be up for making these changes?

---

The main use case I have for this decorator is being able to pass the args forward either to another call to Fire (or to another command line arg parsing system, like argparse etc). Is that similar to how you intend to use it, or did you have something else in mind?

[link89]

We'll want a decorator that causes Fire to still parse args for named and positional args and kwargs, but to pass any remaining args as *varargs unparsed.

This will behave the same for the example in your test case (which only has *varargs, no additional args), but would behave differently if there were other args to the function.

I thought about this option, but it would require some changes inside _ParseKeywordArgs and _ParseArgs and make things complicated.
Besides it is hard to achieve, I doubt that users really want fire to deal with some of the parameters, since they can use class or chain mode if they need to. For example

class Cmd:

  def read(self, file: str, fmt: str):
    ...
    return self

  @PassThrough
  def process(self, *args):
    ...

Then use it as cmd read path/to/file --fmt mp4 - process -i mpeg -f hls index.m3u8

[dbieber]

Besides it is hard to achieve, I doubt that users really want fire to deal with some of the parameters, since they can use class or chain mode if they need to. For example

The example you've given has the end user responsible for calling process. What I have in mind is for the developer of a CLI to be able to use Fire from within their CLI. Here's a toy example.

def launch_flux_capacitor(): ...

def launch_number_cruncher(number1, number2, crunch_volume=9000): ...

@fire.decorators.ArgsPassthrough
def start(use_flux_capacitor, use_number_cruncher, *number_cruncher_args):
  if use_flux_capacitor:
    launch_flux_capacitor()
  if use_number_cruncher:
    fire.Fire(launch_number_cruncher, command=number_cruncher_args)

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

launcher --use_flux_capacitor=True --use_number_cruncher=True 10 20 --crunch_volume=300

In this toy example, number_cruncher_args == ["10", "20", "--crunch_volume=300"].

[link89]

What I have in mind is for the developer of a CLI to be able to use Fire from within their CLI.

I am not sure if I understand correctly, but I think they should use chain (which I believe is the most genius design of Fire) in such use case

class Launcher:
  def __init__(self,  use_flux_capacitor, use_number_cruncher):
     self.ufc = use_flux_capacitor
     self.unc = use_number_cruncher

  @ArgsPassThrought
  def run(self, *raw_args):
     if self.ufc:
        launch_flux_capacitor()
      if self.unc:
        fire.Fire(launch_number_cruncher, command=number_cruncher_args)

And then he should be able to calling this way

launcher --use_flux_capacitor=True --use_number_cruncher=True  - run 10 20 --crunch_volume=300

[link89]

I've taken another look at this issue recently and believe that a simpler design may be more sensible.

Consider the following code:

@PassThrough
def my_cmd(a, b, *args):
    print(a, b, *args)

If the user invokes it as follows:

my_cmd --a=1 b c -d --e=10

With a simple design where all arguments are passed through, the output would naturally be:

--a=1, b, c, -d, --e=10

However, if we opt for partial parsing, it introduces ambiguity and becomes challenging to implement.
The problem is that during the parsing process, --a=1, -d, --e=10 would all be handled.
Then, the parser must discern that only 'a' should be consumed in a key-value format.
The rest of the inputs should remain unchanged.
Yet, when the parser encounters *args, it expects plain values (no flag),
so only 'c' is valid, while -d, --e=10 are invalid for *args.
We would need to expend significant effort to alter the current parser's behavior.

Supporting a scenario where some arguments are parsed and others are passed through would be quite complex and confusing.
A complete pass-through is a reasonable scenario, allowing users to partially parse arguments using the chain method.
Therefore, at least for now, partial pass-through is unnecessary

My recent work involved using this feature to create an internal tool that determines if a command has timed out based on whether a file has been updated and such use case is the main reason I suggest and implement this feature.

class SuperTimeout:
  def __init__(self):
     ...
  def monitor_file(self, file: str, timeout=10):
     ...

  @PassThrough
  def run(self, cmd, *raw_args):
     ...
     subprocess.call(cmd, raw_args)
     ...

This tool would be used by the users as follows:

stimeout monitor_file video.mp4 --timeout=30 - run ffmpeg -i rtsp -o video.mp4

[dbieber]

Using subprocess.call is a great motivating example -- much clearer than calling fire.Fire a second time. 🙏

In your example, I think you intend for cmd to be "ffmpeg" and raw_args to be ["-i", "rtsp", "-o", "video.mp4"]. As a small note, I don't think subprocess.call("ffmpeg", ["-i", "rtsp", "-o", "video.mp4"]) is the right call -- I think what you actually want is subprocess.call(["ffmpeg", "-i", "rtsp", "-o", "video.mp4"]).

My leaning is still toward having a PassThrough decorator that only applies to *varargs. I'd also be open to also having a more flexible one that can apply to all args if we can come up with good semantics for it, but that might be tricky; more on that below.

Here's an even more minimal example showing why I think having an ArgvPassThrough decorator that only applies to *varargs would be valuable.

@ArgvPassThrough
def call_n_times(n: int, *cmd):
  for i in range(n):
    subprocess.call(cmd)

fire.Fire(call_n_times)

example.py --n=2 echo hi 5

The way this would work: *cmd would receive all args as strings (up to any separating -) after everything else has been parsed (including any positional args, named args, **kwargs).

---

Next let's consider how a PassThrough decorator that applies to all arguments would behave in the presence of named args and **kwargs. Consider this code (silly example):

@PassThrough
def call_n_times(n: int, *cmd, style="bold", **kwargs):
  verbose = kwargs.pop("verbose", None)
  for i in range(n):
    if verbose:
      print(apply_style(n, style))
    subprocess.call(cmd)

Consider this call: example.py --n=2 echo hi 5 --verbose
Does cmd become ["echo", "hi", "5", "--verbose"] and kwargs becomes {}?
Or does cmd become ["echo", "hi", "5"] and kwargs becomes {"verbose": True}?

If the decorator had the ArgvPassThrough semantics, it would clearly be the latter. With the semantics you're proposing, I think it would be the former, and there would be no way for a user to provide values for kwargs. I don't want to strand the end user in this way. This is why I'm leaning in favor of the ArgsPassThrough approach here. Let me know what you think.

(Edit: well, another glaring issue with my second example that I overlooked is that n would be passed as a string (if I'm understanding the proposal correctly) whereas the fn wants an int! I am supportive of having decorators that control how arbitrary arguments get parsed. In fact we have such decorators already. @fire.decorators.SetParseFn(str) already does this; but it doesn't change what gets passed to *argv vs what is retained unparsed.)

[link89]

My leaning is still toward having a PassThrough decorator that only applies to *varargs.

I get your point but it is hard for me to implment it ,and there are some edge cases may make things over complicated, so I choose to have PassThough to pass all arguments as their original form.

For this example,

@ArgvPassThrough
def call_n_times(n: int, *cmd):
  for i in range(n):
    subprocess.call(cmd)
fire.Fire(call_n_times)

example.py --n=2 echo hi 5

I would suggest the user to implement this way instead

class CallNTimes:
  def __init__(self, n: int):
    self.n = n

 @PassThrough
  def __call__(self, *cmd):
    for in in range(self.n):
      subprocess.call(cmd)

fire.Fire(CallNTimes)

example.py  --n=2 - echo hi 5

And the same for this case

@PassThrough
def call_n_times(n: int, *cmd, style="bold", **kwargs):
  verbose = kwargs.pop("verbose", None)
  for i in range(n):
    if verbose:
      print(apply_style(n, style))
    subprocess.call(cmd)

as

class CallNTimes:
  def __init__(self, n: int, style="bold", **kwargs):
    self.n = n
    self.style = style
    self.kwargs = kwargs
  
  @PassThrough
  def __call__(self, *cmd):
     verbose = self.kwargs.pop("verbose", None)
    for i in range(self.n):
      if verbose:
        print(apply_style(self.n, self.style))
      subprocess.call(cmd)

So to me have PassThrough to apply to only *vargs may be something nice to have but not required,
as the chain pattern can be useful when someone need it.

Besides, if someone implement the PassThrough as you suggest in the future,
it will be backward compatible with the current one.

@fire.decorators.SetParseFn(str) already does this; but it doesn't change what gets passed to *argv vs what is retained unparsed.)

I am afraid it doesn't work as what I expect.

# x.py
import fire  # v0.6.0

@fire.decorators.SetParseFn(str)
def x(*args):
    print(*args)

fire.Fire(x)

python x.py ffmpeg -i input -o output.mp4
ffmpeg
ERROR: Could not consume arg: -i
Usage: x.py ffmpeg -

For detailed information on this command, run:
  x.py ffmpeg - --help

[link89]

I've reviewed the code related to the parser. Currently, it parses the arguments first and then constructs args and kwargs based on the method signature.

To achieve the functionality you described, it would require adjusting the parser's workflow: it would need to match the parameters based on the method signature.

Thus, I don't think it's the best time to make this change. With the Fire project planning to drop support for Python 2, Fire could consider a complete rewirte of the parser based on the type annotations provided by Python >= 3.5. Implementing your desired pattern at that time would likely be a better choice.

For now, I'd prefer PassThrough to be accepted in its current form. The related code isn't complex and won't impact other functionalities. We can continue to support it until a major version of Fire overhauls the parser and deprecates it afterward.

[link89]

Hi, do you think this PR has a chance of being merged? Achieving your vision seems somewhat challenging at the moment. Implementing just the PassThrough mode would be faster and safer, as this feature is designed to be opt-in, which can avoid impacting other functionalities.