po 19. 8. 2024 v 22:40 odesílatel Crystal Wood <crwood@xxxxxxxxxx> napsal:
>
> 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)
>
The bug in commit 56c7cf63942d is that the "return grparser" command
is indented one tab more than intended (no pun intended), putting it
inside the for loop iterating over list(opts.items()). This leads to
only the first module's arguments to be applied in the case of both
measurements and loads, since the return exists from the loop before
the rest can be processed.
> >
> > This patch reworks the overall group mechanism a little bit to restore
> > these menus.
>
> Menus?
>
The module command-line options, as defined by ModuleParameters()
module-level functions e.g. in rteval/modules/loads/hackbench.py.
> > + # 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
>
The super() call is there to get the parent class. It should rather be
RtEvalModules.SetupModuleOptions(self, parser) so that the parent
constructor does not get called again redundantly.
I suggest reverting this commit and going back to my original
implementation, which does not have the issues you are pointing out
here (measurement options outside of measurement modules and
overriding the grparser variable). We will discuss this offline and
give an update soon.
Tomas
