Merge pull request #2392 from ekouts/bugfix/var_options_type

[bugfix] Automatically convert numeric configuration parameters when set from the command line or the environment

Author: Vasileios Karakasis

reframe/core/pipeline.py

@@ -1518,8 +1518,7 @@ def _clone_to_stagedir(self, url):
         self.logger.debug(f'Cloning URL {url} into stage directory')
         osext.git_clone(
             self.sourcesdir, self._stagedir,
-            # FIXME: cast to float explicitly due to GH #2246
-            timeout=float(rt.runtime().get_option('general/0/git_timeout'))
+            timeout=rt.runtime().get_option('general/0/git_timeout')
         )
 
     @final

reframe/frontend/argparse.py

@@ -82,7 +82,7 @@ def __getattr__(self, name):
         if name not in self.__option_map:
             return ret
 
-        envvar, _, action = self.__option_map[name]
+        envvar, _, action, arg_type = self.__option_map[name]
         if ret is None and envvar is not None:
             # Try the environment variable
             envvar, *delim = envvar.split(maxsplit=2)
@@ -99,6 +99,14 @@ def __getattr__(self, name):
                         raise ValueError(
                             f'environment variable {envvar!r} not a boolean'
                         ) from None
+                elif action == 'store' and arg_type != str:
+                    try:
+                        ret = arg_type(ret)
+                    except ValueError as err:
+                        raise ValueError(
+                            f'cannot convert environment variable {envvar!r} '
+                            f'to {arg_type.__name__!r}'
+                        ) from err
 
         return ret
 
@@ -107,7 +115,7 @@ def update_config(self, site_config):
         namespace'''
         errors = []
         for option, spec in self.__option_map.items():
-            _, confvar, action = spec
+            confvar, action = spec[1:3]
             if action == 'version' or confvar is None:
                 continue
 
@@ -174,7 +182,8 @@ def add_argument(self, *flags, **kwargs):
         self._option_map[opt_name] = (
             kwargs.get('envvar', None),
             kwargs.get('configvar', None),
-            kwargs.get('action', 'store')
+            kwargs.get('action', 'store'),
+            kwargs.get('type', str)
         )
         # Remove envvar and configvar keyword arguments and force dest
         # argument, even if we guessed it, in order to guard against changes

reframe/frontend/cli.py

@@ -391,11 +391,11 @@ def main():
     run_options.add_argument(
         '--max-retries', metavar='NUM', action='store', default=0,
         help='Set the maximum number of times a failed regression test '
-             'may be retried (default: 0)'
+             'may be retried (default: 0)', type=int
     )
     run_options.add_argument(
         '--maxfail', metavar='NUM', action='store', default=sys.maxsize,
-        help='Exit after first NUM failures'
+        help='Exit after first NUM failures', type=int
     )
     run_options.add_argument(
         '--mode', action='store', help='Execution mode to use'
@@ -527,8 +527,10 @@ def main():
         dest='git_timeout',
         envvar='RFM_GIT_TIMEOUT',
         configvar='general/git_timeout',
+        action='store',
         help=('Timeout in seconds when checking if the url is a '
-              'valid repository.')
+              'valid repository.'),
+        type=float
     )
     argparser.add_argument(
         dest='graylog_server',
@@ -568,7 +570,8 @@ def main():
         envvar='RFM_PIPELINE_TIMEOUT',
         configvar='general/pipeline_timeout',
         action='store',
-        help='Timeout for advancing the pipeline'
+        help='Timeout for advancing the pipeline',
+        type=float
     )
     argparser.add_argument(
         dest='remote_detect',
@@ -1172,26 +1175,14 @@ def module_unuse(*paths):
                 parsed_job_options.append(f'--{optstr} {valstr}')
 
         exec_policy.sched_options = parsed_job_options
-        try:
-            max_retries = int(options.max_retries)
-        except ValueError:
-            raise errors.ConfigError(
-                f'--max-retries is not a valid integer: {max_retries}'
-            ) from None
-
-        try:
-            max_failures = int(options.maxfail)
-            if max_failures < 0:
-                raise errors.ConfigError(
-                    f'--maxfail should be a non-negative integer: '
-                    f'{options.maxfail!r}'
-                )
-        except ValueError:
+        if options.maxfail < 0:
             raise errors.ConfigError(
-                f'--maxfail is not a valid integer: {options.maxfail!r}'
-            ) from None
+                f'--maxfail should be a non-negative integer: '
+                f'{options.maxfail!r}'
+            )
 
-        runner = Runner(exec_policy, printer, max_retries, max_failures)
+        runner = Runner(exec_policy, printer, options.max_retries,
+                        options.maxfail)
         try:
             time_start = time.time()
             session_info['time_start'] = time.strftime(

reframe/frontend/executors/policies.py

@@ -339,10 +339,6 @@ def exit(self):
                     'general/0/pipeline_timeout'
                 )
 
-                # FIXME: Always convert due to #GH 2246
-                if timeout is not None:
-                    timeout = float(timeout)
-
                 self._advance_all(self._current_tasks, timeout)
                 if self._pipeline_statistics:
                     num_retired = len(self._retired_tasks)

unittests/test_argparser.py

@@ -103,6 +103,10 @@ def extended_parser():
         dest='keep_stage_files', action='store_true',
         envvar='RFM_KEEP_STAGE_FILES', configvar='general/keep_stage_files'
     )
+    parser.add_argument(
+        '--git-timeout', envvar='RFM_GIT_TIMEOUT', action='store',
+        configvar='general/git_timeout', type=float
+    )
     foo_options.add_argument(
         '--timestamp', action='store',
         envvar='RFM_TIMESTAMP_DIRS', configvar='general/timestamp_dirs'
@@ -154,7 +158,8 @@ def test_option_with_config(default_exec_ctx, extended_parser, tmp_path):
             'RFM_TIMESTAMP': '%F',
             'RFM_NON_DEFAULT_CRAYPE': 'yes',
             'RFM_MODULES_PRELOAD': 'a,b,c',
-            'RFM_KEEP_STAGE_FILES': 'no'
+            'RFM_KEEP_STAGE_FILES': 'no',
+            'RFM_GIT_TIMEOUT': '0.3'
     }):
         site_config = rt.runtime().site_config
         options = extended_parser.parse_args(
@@ -167,6 +172,7 @@ def test_option_with_config(default_exec_ctx, extended_parser, tmp_path):
         assert site_config.get('systems/0/prefix') == str(tmp_path)
         assert site_config.get('general/0/colorize') is False
         assert site_config.get('general/0/keep_stage_files') is False
+        assert site_config.get('general/0/git_timeout') == 0.3
 
         # Defaults specified in parser override those in configuration file
         assert site_config.get('systems/0/stagedir') == '/foo'
@@ -175,8 +181,9 @@ def test_option_with_config(default_exec_ctx, extended_parser, tmp_path):
 def test_option_envvar_conversion_error(default_exec_ctx, extended_parser):
     with rt.temp_environment(variables={
             'RFM_NON_DEFAULT_CRAYPE': 'foo',
+            'RFM_GIT_TIMEOUT': 'non-float'
     }):
         site_config = rt.runtime().site_config
         options = extended_parser.parse_args(['--nocolor'])
         errors = options.update_config(site_config)
-        assert len(errors) == 1
+        assert len(errors) == 2

unittests/test_cli.py

@@ -781,8 +781,8 @@ def test_maxfail_invalid_option(run_reframe):
     )
     assert 'Traceback' not in stdout
     assert 'Traceback' not in stderr
-    assert "--maxfail is not a valid integer: 'foo'" in stdout
-    assert returncode == 1
+    assert "--maxfail: invalid int value: 'foo'" in stderr
+    assert returncode == 2
 
 
 def test_maxfail_negative(run_reframe):
@@ -793,7 +793,7 @@ def test_maxfail_negative(run_reframe):
     )
     assert 'Traceback' not in stdout
     assert 'Traceback' not in stderr
-    assert "--maxfail should be a non-negative integer: '-2'" in stdout
+    assert "--maxfail should be a non-negative integer: -2" in stdout
     assert returncode == 1