Address review comments

* Fix some docstrings
* Add some missing input checks

perfmetrics/scripts/testing_on_gke/examples/dlio/run_tests.py

@@ -27,18 +27,19 @@
 import dlio_workload
 
 
+_DEFAULT_GCSFUSE_MOUNT_OPTIONS = 'implicit-dirs'
+
+
 def run_command(command: str):
   """Runs the given string command as a subprocess."""
   result = subprocess.run(command.split(' '), capture_output=True, text=True)
   print(result.stdout)
   print(result.stderr)
 
 
-DEFAULT_GCSFUSE_MOUNT_OPTIONS = 'implicit-dirs'
-
-
-def escapeCommasInString(gcsfuseMountOptions: str) -> str:
-  return gcsfuseMountOptions.replace(',', '\,')
+def escapeCommasInString(unescapedStr: str) -> str:
+  """Returns equivalent string with ',' replaced with '\,' ."""
+  return unescapedStr.replace(',', '\,')
 
 
 def createHelmInstallCommands(
@@ -47,10 +48,10 @@ def createHelmInstallCommands(
     gcsfuseMountOptions: str,
     machineType: str,
 ):
-  """Create helm install commands for the given set of dlioWorkload objects."""
+  """Create helm install commands for the given dlioWorkload objects."""
   helm_commands = []
   if not gcsfuseMountOptions:
-    gcsfuseMountOptions = DEFAULT_GCSFUSE_MOUNT_OPTIONS
+    gcsfuseMountOptions = _DEFAULT_GCSFUSE_MOUNT_OPTIONS
   for dlioWorkload in dlioWorkloads:
     for batchSize in dlioWorkload.batchSizes:
       commands = [
@@ -109,7 +110,11 @@ def main(args) -> None:
   )
   parser.add_argument(
       '--instance-id',
-      help='unique string ID for current test-run',
+      metavar='A unique string ID to represent the test-run',
+      help=(
+          'Set to a unique string ID for current test-run. Do not put spaces'
+          ' in it.'
+      ),
       required=True,
   )
   parser.add_argument(
@@ -119,7 +124,7 @@ def main(args) -> None:
           'GCSFuse mount-options, in JSON stringified format, to be set for the'
           ' scenario gcsfuse-generic.'
       ),
-      required=True,
+      required=False,
   )
   parser.add_argument(
       '--machine-type',
@@ -136,5 +141,11 @@ def main(args) -> None:
           ' not actually run them.'
       ),
   )
+
   args = parser.parse_args()
+  if ' ' in args.instance_id:
+    raise Exception('Argument --instance-id contains space in it')
+  if len(args.machine_type) == 0 or str.isspace(args.machine_type):
+    raise Exception('Argument --machine-type is empty or only spaces')
+
   main(args)

perfmetrics/scripts/testing_on_gke/examples/fio/run_tests.py

@@ -26,30 +26,31 @@
 import fio_workload
 
 
+_DEFAULT_GCSFUSE_MOUNT_OPTIONS = 'implicit-dirs'
+
+
 def run_command(command: str):
   """Runs the given string command as a subprocess."""
   result = subprocess.run(command.split(' '), capture_output=True, text=True)
   print(result.stdout)
   print(result.stderr)
 
 
-DEFAULT_GCSFUSE_MOUNT_OPTIONS = 'implicit-dirs'
-
-
-def escapeCommasInString(gcsfuseMountOptions: str) -> str:
-  return gcsfuseMountOptions.replace(',', '\,')
+def escapeCommasInString(unescapedStr: str) -> str:
+  """Returns equivalent string with ',' replaced with '\,' ."""
+  return unescapedStr.replace(',', '\,')
 
 
 def createHelmInstallCommands(
     fioWorkloads: set,
     instanceId: str,
     gcsfuseMountOptions: str,
     machineType: str,
-):
-  """Create helm install commands for the given set of fioWorkload objects."""
+) -> list:
+  """Creates helm install commands for the given fioWorkload objects."""
   helm_commands = []
   if not gcsfuseMountOptions:
-    gcsfuseMountOptions = DEFAULT_GCSFUSE_MOUNT_OPTIONS
+    gcsfuseMountOptions = _DEFAULT_GCSFUSE_MOUNT_OPTIONS
   for fioWorkload in fioWorkloads:
     for readType in fioWorkload.readTypes:
       commands = [
@@ -110,7 +111,11 @@ def main(args) -> None:
   )
   parser.add_argument(
       '--instance-id',
-      help='unique string ID for current test-run',
+      metavar='A unique string ID to represent the test-run',
+      help=(
+          'Set to a unique string ID for current test-run. Do not put spaces'
+          ' in it.'
+      ),
       required=True,
   )
   parser.add_argument(
@@ -120,7 +125,7 @@ def main(args) -> None:
           'GCSFuse mount-options, in JSON stringified format, to be set for the'
           ' scenario gcsfuse-generic.'
       ),
-      required=True,
+      required=False,
   )
   parser.add_argument(
       '--machine-type',
@@ -137,5 +142,11 @@ def main(args) -> None:
           ' not actually run them.'
       ),
   )
+
   args = parser.parse_args()
+  if ' ' in args.instance_id:
+    raise Exception('Argument --instance-id contains space in it')
+  if len(args.machine_type) == 0 or str.isspace(args.machine_type):
+    raise Exception('Argument --machine-type is empty or only spaces')
+
   main(args)