On Sun, 2024-08-18 at 15:42 -0400, John Kacur wrote:
> Commit 56c7cf63942d rteval: Allow arguments specific to module group
>
> intended to allow group options to the overall measurement modules without
> applying to the load modules. It inadvertently disabled options for most
> load modules such as hackbench and kcompile.
How did that happen and how does this fix it?
(not saying it doesn't, just that clarifying that would help with reviewing
and not introducing something similar in the future)
>
> This patch reworks the overall group mechanism a little bit to restore
> these menus.
Menus?
> + # Set up options for measurement modules only
> + if self.__modtype == 'measurement':
> + grparser.add_argument(f'--{self.__modtype}-run-on-isolcpus',
> + dest =
> f'{self.__modtype}___run_on_isolcpus',
> + action = "store_true",
> + default =
> config.GetSection("measurement").setdefault("run-on-isolcpus",
> "false").lower() == "true",
> + help = "Include isolated CPUs in
> default cpulist")
You already know it's measurement in this section, so why {self.__modtype}?
Why does this need to be here rather than in rteval/modules/measurement.py?
> for (modname, mod) in list(self.__modsloaded.items()):
> opts = mod.ModuleParameters()
> if len(opts) == 0:
> @@ -296,7 +310,7 @@ reference from the first import"""
> # Ignore if a section is not found
> cfg = None
>
> - modgrparser = parser.add_argument_group(f"Options for the
> {shortmod} module")
> + grparser = parser.add_argument_group(f"Options for the
> {shortmod} module")
> for (o, s) in list(opts.items()):
> descr = 'descr' in s and s['descr'] or ""
> metavar = 'metavar' in s and s['metavar'] or None
> @@ -311,7 +325,7 @@ reference from the first import"""
> default = 'default' in s and s['default'] or None
>
>
> - modgrparser.add_argument(f'--{shortmod}-{o}',
> + grparser.add_argument(f'--{shortmod}-{o}',
> dest=f"{shortmod}___{o}",
> action='store',
> help='%s%s' % (descr,
> @@ -319,8 +333,6 @@ reference from the first import"""
> default=default,
> metavar=metavar)
Wouldn't "modparser" be more accurate, since this isn't for the entire
group?
Or we could reuse the mechanism for both group and module level (maybe
s/ModuleParameters/Parameters/) if it's not introducing more complexity than
it eliminates.
> diff --git a/rteval/modules/measurement/__init__.py
> b/rteval/modules/measurement/__init__.py
> index 9314d1cb6bbc..44708ce0b035 100644
> --- a/rteval/modules/measurement/__init__.py
> +++ b/rteval/modules/measurement/__init__.py
> @@ -29,17 +29,7 @@ class MeasurementModules(RtEvalModules):
>
> def SetupModuleOptions(self, parser):
> "Sets up all the measurement modules' parameters for the option
> parser"
> - grparser = super().SetupModuleOptions(parser)
> -
> - # Set up options specific for measurement module group
> - grparser.add_argument("--measurement-run-on-isolcpus",
> - dest="measurement___run_on_isolcpus",
> - action="store_true",
> -
> default=self._cfg.GetSection("measurement").setdefault("run-on-isolcpus",
> "false").lower()
> - == "true",
> - help="Include isolated CPUs in default
> cpulist")
> - grparser.add_argument('--idle-set',
> dest='measurement___idlestate', metavar='IDLESTATE',
> - default=None, help='Idle state depth to set on
> cpus running measurement modules')
> + super().SetupModuleOptions(parser)
Why is this method still here if all it does is call super?
-Crystal
