Allow sys.argv and args with LightningCLI for testing

[Erotemic]

Outline & Motivation

I currently am forced to inherit from LightningCLI and overload parse_arguments to disable an error when you call LightningCLI with args and sys.argv is populated.

class LightningCLI_Extension(LightningCLI):
    ...

    def parse_arguments(self, parser: LightningArgumentParser, args) -> None:
        """Parses command line arguments and stores it in ``self.config``."""
        import sys
        if args is not None and len(sys.argv) > 1:
            # Please let us shoot ourselves in the foot.
            import warnings
            warnings.warn(
                "LightningCLI's args parameter is intended to run from within Python like if it were from the command "
                "line. To prevent mistakes it is not allowed to provide both args and command line arguments, got: "
                f"sys.argv[1:]={sys.argv[1:]}, args={args}."
            )
        if isinstance(args, (dict, Namespace)):
            self.config = parser.parse_object(args)
        else:
            self.config = parser.parse_args(args)

The reason I'm doing this is testing. I have automated tests and doctests that ensure our LightningCLI scripts play nice with our models / dataloaders. However, when I run testing software like pytest or xdoctest, the sys.argv list is populated, but in my test I call LightningCLI programatically with a dictionary config.

Pitch

It would be nice if this was either changed to a warning, or allow the user to pass a flag that makes the error a warning.

If the maintainers think this is a good idea I can write the PR.

Additional context

No response

cc @Borda @justusschock @awaelchli @carmocca @mauvilsa

[carmocca]

This is not necessary. You can patch sys.argv which is also what we do in our pytest tests: https://github.com/Lightning-AI/lightning/blob/2844e9e2461e9f48a9b18759a58bbdc2ac4b5284/tests/tests_pytorch/test_cli.py#L1534-L1535

[Erotemic]

I don't like monkeypatching when I can get away with not doing it.

Regardless, patching doesn't work / make sense for all testing frameworks like xdoctest.

[carmocca]

If circumventing this exception is only relevant for your tests, why not use this extension class you already wrote in your test suite?

def parse_arguments(...):
   try:
       super().parse_arguments():
   except ValueError:
       # we are testing, ignore exception
       do_something()

I would rather not adapt the implementation for a specific testing requirement. It is preferred that tests are adapted instead (for example by monkeypatching).
Additionally warnings are often ignored by users, so that's why I prefer to keep it as an exception as this shouldn't happen during non-test execution

[Erotemic]

The main place it is relevant is tests. There is also the case that if you invoke IPython with args then it will also error. This is not the case yet, but I do envision a system where other tools (that might have their own sys.argv) are invoking our fit method (implemented via LightningCLI) programatically. Of course I can just keep my patched version, but as I've said, I prefer to not patch whenever possible.

What I'd like to know is: is there a technical reason why sys.argv should not be populated when running with args? For instance, when running with different strategies, will there be issues if sys.argv is not empty? Or is it the case that when args is specified sys.argv might as well be empty?

If removing the exception actually breaks something, then my comment ends here, but if not then I'll make this point:

For me, running into this error was surprising and took me a while to figure out how to work around it. Was this feature introduced because users were making mistakes thinking that sys.argv would be respected even if args was given? In other words: is this a case of preemptiveness? Or is this really stopping people from making mistakes. My arm-chair thought is that: if you are passing in args you know what you are doing in some respect and expect sys.argv to be ignored. My goal in raising this issue here is to prevent others from running into the same trouble I did. But this is all under an assumption that this exception really isn't helping anyone, so if it is, I'm happy to concede the point, but if it was just put in there defensively, the decision should be reconsidered.

[carmocca]

@mauvilsa Do you have any thoughts on the above? #14596 is the PR that introduced this constraint

[mauvilsa]

The reason why this was added is only this comment #14596 (comment). I am neutral about removing this.