Address reviewer feedback

DRY out upload method. Moved uploader type/uri logic into uploader and
removed workspace name from upload method. Fix minor issues.

lib/ramble/ramble/cmd/results.py

@@ -7,89 +7,54 @@
 # except according to those terms.
 
 import llnl.util.tty as tty
+import json
 
-from ramble.config import ConfigError
 import ramble.experimental.uploader
 
-
 description = "take actions on experiment results"
-section = "workspaces"
+section = "results"
 level = "short"
 
-subcommands = [
-    'file_upload',
-]
-
-#: Dictionary mapping subcommand names and aliases to functions
-subcommand_functions = {}
-
-
-def results_file_upload_setup_parser(subparser):
-    subparser.add_argument(
-        'filename', metavar='filename',
-        help='path of file to upload')
 
+def setup_parser(subparser):
+    sp = subparser.add_subparsers(metavar='SUBCOMMAND',
+                                  dest='results_command')
 
-def results_file_upload(args):
-    """Imports Ramble experiment results from JSON file and uploads them."""
-    imported_results = ramble.experimental.uploader.results_file_import(args.filename)
-
-    if ramble.config.get('config:upload'):
-        # Read upload type and push it there
-        if ramble.config.get('config:upload:type') == 'BigQuery':  # TODO: enum?
-            try:
-                formatted_data = ramble.experimental.uploader.format_data(imported_results)
-            except KeyError:
-                tty.die("Error parsing file: Does not contain valid data.")
-
-            # TODO: strategy object?
-            uploader = ramble.experimental.uploader.BigQueryUploader()
+    # Upload
+    upload_parser = sp.add_parser('upload', help=results_upload.__doc__)
+    upload_parser.add_argument(
+        'filename', help='path of file to upload')
 
-            # Read workspace name from results for uploader, or use default.
-            try:
-                workspace_name = formatted_data[0].workspace_name
-            except KeyError:
-                workspace_name = "Default Workspace"
 
-            uri = ramble.config.get('config:upload:uri')
-            if not uri:
-                tty.die('No upload URI (config:upload:uri) in config.')
+def results_upload(args):
+    """Imports Ramble experiment results from JSON file and uploads them as
+    specified in the upload block of Ramble's config file."""
+    imported_results = import_results_file(args.filename)
 
-            tty.msg('Uploading Results to ' + uri)
-            uploader.perform_upload(uri, workspace_name, formatted_data)
-        else:
-            raise ConfigError("Unknown config:upload:type value")
+    ramble.experimental.uploader.upload_results(imported_results)
 
-    else:
-        raise ConfigError("Missing correct conifg:upload parameters")
 
+def import_results_file(filename):
+    """
+    Import Ramble experiment results from a JSON file.
+    """
+    tty.debug("File to import:")
+    tty.debug(filename)
 
-def setup_parser(subparser):
-    sp = subparser.add_subparsers(metavar='SUBCOMMAND',
-                                  dest='results_command')
+    imported_file = open(filename)
 
-    for name in subcommands:
-        if isinstance(name, (list, tuple)):
-            name, aliases = name[0], name[1:]
+    try:
+        tty.msg("Import file...")
+        parsed_json_file = json.load(imported_file)
+        # Check if data contains an experiment
+        if parsed_json_file.get('experiments'):
+            return parsed_json_file
         else:
-            aliases = []
-
-        # add commands to subcommands dict
-        function_name = 'results_%s' % name
-        function = globals()[function_name]
-        for alias in [name] + aliases:
-            subcommand_functions[alias] = function
-
-        # make a subparser and run the command's setup function on it
-        setup_parser_cmd_name = 'results_%s_setup_parser' % name
-        setup_parser_cmd = globals()[setup_parser_cmd_name]
-
-        subsubparser = sp.add_parser(
-            name, aliases=aliases, help=setup_parser_cmd.__doc__)
-        setup_parser_cmd(subsubparser)
+            tty.die("Error parsing file: Does not contain valid data to upload.")
+    except ValueError:
+        tty.die("Error parsing file: Invalid JSON formatting.")
 
 
 def results(parser, args):
-    """Look for a function called environment_<name> and call it."""
-    action = subcommand_functions[args.results_command]
-    action(args)
+    action = {'upload': results_upload}
+    action[args.results_command](args)

lib/ramble/ramble/cmd/workspace.py

@@ -26,6 +26,7 @@
 import ramble.workspace
 import ramble.workspace.shell
 import ramble.experiment_set
+import ramble.experimental.uploader
 import ramble.software_environments
 import ramble.util.colors as rucolor
 
@@ -363,7 +364,7 @@ def workspace_analyze(args):
 
     # FIXME: this will fire the analyze logic of twice currently
     if args.upload:
-        ws.upload_results()
+        ramble.experimental.uploader.upload_results(ws.results)
 
 
 def workspace_info_setup_parser(subparser):

lib/ramble/ramble/experimental/uploader.py

@@ -11,20 +11,23 @@
 import sys
 import math
 
+import ramble.config
+from ramble.config import ConfigError
+
 
 default_node_type_val = "Not Specified"
 
 
 class Uploader():
     # TODO: should the class store the base uri?
-    def perform_upload(self, uri, workspace_name, data):
+    def perform_upload(self, uri, data):
         # TODO: move content checking to __init__ ?
         if not uri:
             raise ValueError(
-                "%s requires %s argument." % (self.__class__, uri))
+                f"{self.__class__} requires {uri} argument.")
         if not data:
             raise ValueError(
-                "%s requires %s argument." % (self.__class__, data))
+                f"{self.__class__} requires %{data} argument.")
         pass
 
 
@@ -109,6 +112,31 @@ def determine_node_type(experiment, contexts):
             continue
 
 
+def upload_results(results):
+    if ramble.config.get('config:upload'):
+        # Read upload type and push it there
+        if ramble.config.get('config:upload:type') == 'BigQuery':  # TODO: enum?
+            try:
+                formatted_data = ramble.experimental.uploader.format_data(results)
+            except KeyError:
+                tty.die("Error parsing file: Does not contain valid data to upload.")
+            # TODO: strategy object?
+
+            uploader = BigQueryUploader()
+
+            uri = ramble.config.get('config:upload:uri')
+            if not uri:
+                tty.die('No upload URI (config:upload:uri) in config.')
+
+            tty.msg('Uploading Results to ' + uri)
+            uploader.perform_upload(uri, formatted_data)
+        else:
+            raise ConfigError("Unknown config:upload:type value")
+
+    else:
+        raise ConfigError("Missing correct config:upload parameters")
+
+
 def format_data(data_in):
     """
     Goal: convert results to a more searchable and decomposed format for insertion
@@ -118,7 +146,7 @@ def format_data(data_in):
 
     .. code-block:: text
 
-        { expierment_name:
+        { experiment_name:
             { "CONTEXTS": {
                 "context_name": "FOM_name { unit: "value", "value":value" }
             ...}
@@ -162,27 +190,6 @@ def format_data(data_in):
     return results
 
 
-def results_file_import(filename):
-    """
-    Import Ramble experiment results from a JSON file.
-    """
-    tty.debug("Import file")
-    tty.debug(filename)
-
-    imported_file = open(filename)
-
-    try:
-        tty.msg("Import file...")
-        parsed_json_file = json.load(imported_file)
-        # Check if data contains an experiment
-        if parsed_json_file.get('experiments'):
-            return parsed_json_file
-        else:
-            tty.die("Error parsing file: Does not contain valid data.")
-    except ValueError:
-        tty.die("Error parsing file: Invalid JSON formatting.")
-
-
 class BigQueryUploader(Uploader):
     """Class to handle upload of FOMs to BigQuery
     """
@@ -218,7 +225,7 @@ def chunked_upload(self, table_id, data):
                 return error
         return error
 
-    def insert_data(self, uri: str, workspace_name, results) -> None:
+    def insert_data(self, uri: str, results) -> None:
 
         # It is expected that the user will create these tables outside of this
         # tooling
@@ -254,13 +261,13 @@ def insert_data(self, uri: str, workspace_name, results) -> None:
             else:
                 tty.die("Encountered errors while inserting rows: {}".format(errors))
 
-    def perform_upload(self, uri, workspace_name, results):
-        super().perform_upload(uri, workspace_name, results)
+    def perform_upload(self, uri, results):
+        super().perform_upload(uri, results)
 
         # import spack.util.spack_json as sjson
         # json_str = sjson.dump(results)
 
-        self.insert_data(uri, workspace_name, results)
+        self.insert_data(uri, results)
 
     # def get_max_current_id(uri, table):
         # TODO: Generating an id based on the max in use id is dangerous, and

lib/ramble/ramble/test/cmd/results.py

@@ -11,7 +11,6 @@
 
 import ramble.paths
 import ramble.cmd.results
-import ramble.experimental.uploader
 
 INPUT_DATA = py.path.local(ramble.paths.test_path).join('data', 'results_upload')
 
@@ -21,7 +20,7 @@
     [
         (
             py.path.local(INPUT_DATA).join('test1_empty_experiments.json'),
-            'Error parsing file: Does not contain valid data.',
+            'Error parsing file: Does not contain valid data to upload.',
         ),
         (
             py.path.local(INPUT_DATA).join('test2_not_json.txt.json'),
@@ -35,6 +34,6 @@
 )
 def test_file_import_rejects_invalid_files(filename, expected_output, capsys):
     with pytest.raises(SystemExit):
-        ramble.experimental.uploader.results_file_import(filename)
+        ramble.cmd.results.import_results_file(filename)
         captured = capsys.readouterr()
         assert expected_output in captured

lib/ramble/ramble/workspace/workspace.py

@@ -33,8 +33,6 @@
 import ramble.keywords
 import ramble.software_environments
 from ramble.mirror import MirrorStats
-from ramble.config import ConfigError
-import ramble.experimental.uploader
 
 import spack.util.spack_yaml as syaml
 import spack.util.spack_json as sjson
@@ -1067,27 +1065,6 @@ def write_json_results(self):
             sjson.dump(self.results, f)
         return out_file
 
-    def upload_results(self):
-        if ramble.config.get('config:upload'):
-            # Read upload type and push it there
-            if ramble.config.get('config:upload:type') == 'BigQuery':  # TODO: enum?
-                formatted_data = ramble.experimental.uploader.format_data(self.results)
-
-                # TODO: strategy object?
-                uploader = ramble.experimental.uploader.BigQueryUploader()
-
-                uri = ramble.config.get('config:upload:uri')
-                if not uri:
-                    tty.die('No upload URI (config:upload:uri) in config.')
-
-                tty.msg('Uploading Results to ' + uri)
-                uploader.perform_upload(uri, self.name, formatted_data)
-            else:
-                raise ConfigError("Unknown config:upload:type value")
-
-        else:
-            raise ConfigError("Missing correct conifg:upload parameters")
-
     def default_results(self):
         res = {}
 

share/ramble/ramble-completion.bash

@@ -600,16 +600,16 @@ _ramble_results() {
     then
         RAMBLE_COMPREPLY="-h --help"
     else
-        RAMBLE_COMPREPLY="file_upload"
+        RAMBLE_COMPREPLY="upload"
     fi
 }
 
-_ramble_results_file_upload() {
+_ramble_results_upload() {
     if $list_options
     then
         RAMBLE_COMPREPLY="-h --help"
     else
-        RAMBLE=""
+        RAMBLE_COMREPLY=""
     fi
 }