| Message ID | 20240818194216.299176-2-jkacur@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Changes to supply a customer kernel | expand |
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
po 19. 8. 2024 v 22:40 odesílatel Crystal Wood <crwood@redhat.com> 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
diff --git a/rteval/modules/__init__.py b/rteval/modules/__init__.py index acd6330788e2..eb29db86ce7a 100644 --- a/rteval/modules/__init__.py +++ b/rteval/modules/__init__.py @@ -280,10 +280,24 @@ reference from the first import""" grparser = parser.add_argument_group(f"Group Options for {self.__modtype} modules") grparser.add_argument(f'--{self.__modtype}-cpulist', - dest=f'{self.__modtype}___cpulist', action='store', default="", + dest=f'{self.__modtype}___cpulist', action='store', + default="", help=f'CPU list where {self.__modtype} modules will run', metavar='CPULIST') + # 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") + grparser.add_argument('--idle-set', + dest='measurement___idlestate', + metavar='IDLESTATE', + default=None, + help='Idle state depth to set on cpus running measurement modules') + 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) - return grparser - def InstantiateModule(self, modname, modcfg, modroot=None): """Imports a module and instantiates an object from the modules create() function. 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) def Setup(self, modparams):
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. This patch reworks the overall group mechanism a little bit to restore these menus. Signed-off-by: John Kacur <jkacur@redhat.com> --- rteval/modules/__init__.py | 22 +++++++++++++++++----- rteval/modules/measurement/__init__.py | 12 +----------- 2 files changed, 18 insertions(+), 16 deletions(-)