changes in verdi restapi command (#2384) #2853

waychal · 2019-05-08T12:07:47Z

updated verdi restapi command and its click parameters
removed load_profile functionality from restapi code as it is already handled by verdi command
removed __main__ from restapi code as it is duplicating the functionality provided by 'verdi restapi' command

sphuber

Thanks @waychal some minor comments

sphuber · 2019-05-08T12:11:21Z

aiida/cmdline/commands/cmd_restapi.py

@@ -26,33 +26,44 @@


 @verdi.command('restapi')
-@click.option('-H', '--host', type=click.STRING, default='127.0.0.1', help='the hostname to use')
+@click.option('-h', '--host', type=click.STRING, default='127.0.0.1', help='the hostname to use')


This will be problematic, because -h is already as the short option for the --help flag, which is automatically added by verdi itself. The predefined overridable options in aiida.cmdline.params.options already define an option for the hostname and port. Please use those, so that the same flags will be used all across verdi.

sphuber · 2019-05-08T12:13:43Z

aiida/restapi/run_api.py

-    else:
-        aiida_profile = "default"
+    catch_internal_server = kwargs['catch_internal_server']
+    debug = kwargs["debug"]


Please use single quotes for string literal identifiers

sphuber · 2019-05-08T12:16:49Z

aiida/cmdline/commands/cmd_restapi.py

+    type=click.BOOL,
+    default=False,
+    help='to use WSGI profiler middleware for finding bottlenecks in web application')
+@click.option('--hookup', type=click.BOOL, default=True, help='to hookup app')


For flag-like options, you should use click.option('--hookup', is_flag=True, default=False). A user then only has to specify --hookup without the boolean value. If the flag needs to be on by default and needs to be turned off, you can use click.option('--hookup/--no-hookup', 'hookup', is_flag=True, default=True). This also applies to --wsgi-profile and --debug.

If the flag is off by default and should just be passed to turn it on, like in the case of --debug, you only want to specify the on flag and do not use the double flag version.

sphuber · 2019-05-08T12:18:03Z

aiida/cmdline/commands/cmd_restapi.py

    """
    Run the AiiDA REST API server

    Example Usage:

        \b
        verdi -p <profile_name> restapi --host 127.0.0.5 --port 6789 --config-dir <location of the config.py file>
+        --debug True --wsgi-profile False --hookup True


Don't forget to update this after you have changed the options

sphuber · 2019-05-08T12:20:57Z

aiida/restapi/run_api.py


-    else:
-        aiida_profile = "default"
+    catch_internal_server = kwargs['catch_internal_server']


Probably want to use something like kwargs.pop('catch_internal_server', False) to avoid KeyError if the keyword is not passed in. Here False should then be the default value you want to use

sphuber · 2019-05-08T12:21:55Z

aiida/restapi/run_api.py

-    elif confs.DEFAULT_AIIDA_PROFILE is not None:
-        aiida_profile = confs.DEFAULT_AIIDA_PROFILE
+    # Unpack parameters
+    default_host = kwargs['default_host']


Why are these called default? They are simply the host and port that the user wants to use, no?

sphuber · 2019-05-08T12:22:31Z

aiida/cmdline/commands/cmd_restapi.py

        prog_name='verdi-restapi',
        default_host=host,
        default_port=port,
        default_config=config_dir,
+        debug=debug,
+        wsgi_profile=wsgi_profile,
+        hookup=hookup,


I see that parse_aiida_profile is still being passed, but I don't think it is being used in the run_api function. If true, please remove it

coveralls · 2019-05-08T14:53:21Z

Coverage increased (+0.2%) to 72.731% when pulling 16e4020 on waychal:restapi_cmdline into 8836a8f on aiidateam:develop.

coveralls · 2019-05-08T14:53:21Z

Coverage increased (+0.2%) to 72.653% when pulling d87b35e0c85d72f1a8bfd280df20046efa87962e on waychal:restapi_cmdline into af9fba3 on aiidateam:develop.

coveralls · 2019-05-08T14:53:22Z

Coverage increased (+0.2%) to 72.653% when pulling d87b35e0c85d72f1a8bfd280df20046efa87962e on waychal:restapi_cmdline into af9fba3 on aiidateam:develop.

waychal · 2019-05-08T15:30:02Z

@sphuber I have made the requested changes.

sphuber

You missed a few changes

sphuber · 2019-05-08T15:35:24Z

aiida/cmdline/commands/cmd_restapi.py

@@ -26,34 +26,44 @@


 @verdi.command('restapi')
-@click.option('-H', '--host', type=click.STRING, default='127.0.0.1', help='the hostname to use')
-@click.option('-p', '--port', type=click.INT, default=5000, help='the port to use')
+@click.option('-H', '--hostname', type=click.STRING, default='127.0.0.1', help='Hostname')


please use the predefined aiida.cmdline.params.options.HOSTNAME and aiida.cmdline.params.options.PORT for these. Note you can override the default when you call it. See other verdi commands for examples

I have made changes for this but I am not sure if it is correct. Please let me know if I am missing anything again.

nope, all good 👍

sphuber · 2019-05-08T15:35:58Z

aiida/cmdline/commands/cmd_restapi.py

-        default_host=host,
-        default_port=port,
+        hostname=hostname,
+        port=port,


the same goes for the default_config no? As in, this should just be config

sphuber · 2019-05-08T15:36:09Z

aiida/restapi/run_api.py

+    # Unpack parameters
+    hostname = kwargs['hostname']
+    port = kwargs['port']
+    default_config_dir = kwargs['default_config']


sphuber · 2019-05-09T12:15:41Z

Only your pre-commit is failing. Make sure you have updated your dependencies: pip install -e .[dev_precommit] --upgrade. If you fix those and push, I will approve. Thanks!

- updated verdi restapi command and its click parameters - removed load_profile functionality from restapi code as it is already handled by verdi command - removed __main__ from restapi code as it is duplicating the functionality provided by 'verdi restapi' command

sphuber · 2019-05-09T15:48:06Z

Excellent! Thanks a lot @waychal

waychal requested a review from sphuber May 8, 2019 12:07

sphuber requested changes May 8, 2019

View reviewed changes

waychal force-pushed the restapi_cmdline branch 2 times, most recently from a98bb1e to d87b35e Compare May 8, 2019 14:17

sphuber requested changes May 8, 2019

View reviewed changes

waychal force-pushed the restapi_cmdline branch 2 times, most recently from d662f3f to a438b50 Compare May 9, 2019 09:02

waychal force-pushed the restapi_cmdline branch from a438b50 to 16e4020 Compare May 9, 2019 13:34

sphuber approved these changes May 9, 2019

View reviewed changes

sphuber merged commit e9650cc into aiidateam:develop May 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes in verdi restapi command (#2384) #2853

changes in verdi restapi command (#2384) #2853

waychal commented May 8, 2019

sphuber left a comment

sphuber May 8, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

coveralls commented May 8, 2019 •

edited

Loading

coveralls commented May 8, 2019

coveralls commented May 8, 2019

waychal commented May 8, 2019

sphuber left a comment

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber May 8, 2019

waychal May 9, 2019

sphuber commented May 9, 2019

sphuber commented May 9, 2019

changes in verdi restapi command (#2384) #2853

changes in verdi restapi command (#2384) #2853

Conversation

waychal commented May 8, 2019

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 8, 2019 • edited Loading

coveralls commented May 8, 2019

coveralls commented May 8, 2019

waychal commented May 8, 2019

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented May 9, 2019

sphuber commented May 9, 2019

coveralls commented May 8, 2019 •

edited

Loading