Fixing Example and Adding Keyword Arguments as option for parameters

[JonahKr]

1. This fixes a small typo in the async example
2. Just by adding a few lines of code, it enables the functionality to pass parameters in code. (Issue Allow other way than command line arguments to specify parameters? #15 ) e.g:
   python app = HermesApp("AdviceApp", host = "192.168.178.123", port = 12183)

Not sure if it should be restricted to the existing parameters (or some parameters exclusively) or not. I'm leaning towards no restrictions.

[koenvervloesem]

Thanks! An easy addition, so it seems like a no-brainer. I'll have a look at it soon.

[JonahKr]

One issue which just came into my mind is that we would need to decide which configuration will override in case of assigning different values as cli arguments and keyword arguments.
Mainly this is to be decided by the user but I see an error potential here.

[koenvervloesem]

Ok, I have been playing with this and this looks nice.

So currently the parameters you pass to the HermesApp initialization take precedence over the options on the command line. To me it seems like the other way around is more logical, no?

[JonahKr]

Ok, I have been playing with this and this looks nice.

So currently the parameters you pass to the HermesApp initialization take precedence over the options on the command line. To me it seems like the other way around is more logical, no?

Yes thats right... I initially thought it wouldn't matter because the user has to decide which option to use, but by ranking cli Arguments higher, we actually add a feature for the App developer to set standard parameters which can be overwritten by the user if his settings are different. I will implement a fix.

[JonahKr]

@koenvervloesem i think i fixed the priorities

[maxbachmann]

@JonahKr I missed that you use args_dict only to edit self.args, while my changed version creates a new dict

[JonahKr]

@maxbachmann yes me too.... i guess its fixed now

[maxbachmann]

No it still creates a new object

[maxbachmann]

@JonahKr Here is a better solution:

args = {**kwargs, **vars(parser.parse_args())}
self.args = argparse.Namespace(**args)

I think this makes more clear, that it is changing this Namespace object aswell, which was really easy to miss in the initial solution

[JonahKr]

args = {**kwargs, **vars(parser.parse_args())}
self.args = argparse.Namespace(**args)

I think this makes more clear, that it is changing this Namespace object aswell, which was really easy to miss in the initial solution

my solution did indeed change the Namespace object. (Edit: apparently only if the key already existed) But the CLI-arguments have standard values which therefore causes them to not get overwritten...

Edit: Confusion intensifies.... BUT @koenvervloesem the standard values are a breaking issue

[JonahKr]

maybe we should finish this thread afterwards, close the pull request and create a new one, or otherwise a ton of commits without content thrash up the history

[maxbachmann]

But the CLI-arguments have standard values which therefore causes them to not get overwritten...

thats true (this should have already the same problem in your initial priority change)

[JonahKr]

(this should have already the same problem in your initial priority change

yes but i didn't have the environment to test it and tried to replicate the environment. but now i finished the setup off a rhasspy container on my laptop so i tried it

[koenvervloesem]

Ok I haven't tried to fix this code yet, but because it seems more difficult than I initially thought, I have written some test cases, so we can see for sure which corner cases are failing while trying to fix this. I think I have covered all relevant cases in the test, feel free to add tests if you think I missed something.

As the tests show, the default CLI arguments are indeed problematic:

mocker = <pytest_mock.plugin.MockFixture object at 0x7f382d83aa60>

    def test_arguments_in_init(mocker):
        """Test whether arguments are set up correctly while initializing a HermesApp object."""
        app = HermesApp(
            "Test arguments in init",
            mqtt_client=mocker.MagicMock(),
            host="rhasspy.home",
            port=8883,
            tls=True,
            username="rhasspy-hermes-app",
            password="test",
        )
    
>       assert app.args.host == "rhasspy.home"
E       AssertionError: assert 'localhost' == 'rhasspy.home'
E         - rhasspy.home
E         + localhost

tests/test_arguments.py:59: AssertionError

By the way, @JonahKr:

maybe we should finish this thread afterwards, close the pull request and create a new one, or otherwise a ton of commits without content thrash up the history

Comments won't end up as commits, so no worries :-)

[koenvervloesem]

Ok I have something that seems to work, but it's a bit ugly :-) make test passes, but I have to look at the failing make check.

Can you two review this?

[maxbachmann]

Wouldn't this break when someone passes a default argument value on purpose to the cli?

[koenvervloesem]

You're right, I even thought about it this morning, but forgot this when working on the fix. I have covered this test case now in the tests:

mocker = <pytest_mock.plugin.MockFixture object at 0x7f70d70d8310>

    def test_if_cli_arguments_overwrite_init_arguments(mocker):
        """Test whether arguments from the command line overwrite arguments to a HermesApp object."""
        mocker.patch(
            "sys.argv",
            [
                "rhasspy-hermes-app-test",
                "--host",
                "rhasspy.home",
                "--port",
                "1883",
                "--username",
                "rhasspy-hermes-app",
                "--password",
                "test"
            ],
        )
        app = HermesApp(
            "Test arguments in init",
            mqtt_client=mocker.MagicMock(),
            host="rhasspy.local",
            port=8883,
            username="rhasspy-hermes-app-test",
            password="covfefe",
        )
    
        assert app.args.host == "rhasspy.home"
>       assert app.args.port == 1883
E       AssertionError: assert 8883 == 1883
E        +  where 8883 = Namespace(debug=False, host='rhasspy.home', log_format='[%(levelname)s:%(asctime)s] %(name)s: %(message)s', password='...CERT_REQUIRED', tls_certfile=None, tls_ciphers=None, tls_keyfile=None, tls_version=None, username='rhasspy-hermes-app').port
E        +    where Namespace(debug=False, host='rhasspy.home', log_format='[%(levelname)s:%(asctime)s] %(name)s: %(message)s', password='...CERT_REQUIRED', tls_certfile=None, tls_ciphers=None, tls_keyfile=None, tls_version=None, username='rhasspy-hermes-app') = <rhasspyhermes_app.HermesApp object at 0x7f70d70e22b0>.args

tests/test_arguments.py:91: AssertionError

Still thinking about the best way to fix this...

Just to make sure we're on the same page: when someone passes a default argument value on purpose as a command-line argument, this should take precedence over the value passed to the HermesApp object, right?

[maxbachmann]

yes I think it would be totally confusing when some command-line arguments take precedence while others do not.

[koenvervloesem]

So, apparently argparse can't distinguish between an argument that it assigns its default value because you don't supply it and the same argument that you assign its default value explicitly. At least I haven't found a way. Maybe one of you has more luck finding it?

[maxbachmann]

@koenvervloesem could you add another test for the case that the user provides his own parser, that might both include optional and required arguments, but should apparently follow the same system that cli arguments take precendence over the kwargs

[koenvervloesem]

Thanks, @maxbachmann! I added another test for verifying that it still works when you supply your own argument parser.

If @JonahKr can have a look at the result, I think this is ready to be merged and create a new release :-)

[JonahKr]

I cannot test it atm but the code looks good. If both of you tested it and it works, it's ready to merge in my opinion.

[JonahKr]

But maybe merge it as 1 commit.😂 Because this hellscape of wrong code should never see the light of day again...

[maxbachmann]

But maybe merge it as 1 commit.joy Because this hellscape of wrong code should never see the light of day again...

I agree this should probably simply be squashed when merging