Fix Manifest Group (#235)

This addresses several of the problems noted in #234.

Notably:

* disallow groups named "all"
* disallow duplicate group names, even if in different included manifests
* disallow benchmarks named "all"
* disallow duplicate benchmark names
* clean up the "list_groups" command
* fix how group resolution falls back to the tags

Author: ericsnowcurrently

pyperformance/_manifest.py

@@ -96,10 +96,12 @@ def benchmarks(self):
     @property
     def groups(self):
         names = self._custom_groups()
-        if not names:
-            names = set(self._get_tags())
         return names | {'all', 'default'}
 
+    @property
+    def tags(self):
+        return set(self._get_tags())
+
     @property
     def filename(self):
         return self._raw_filename
@@ -117,25 +119,26 @@ def _add_section_for_file(self, filename, section, data, resolve, seen):
         if resolve is None and filename == DEFAULT_MANIFEST:
             resolve = resolve_default_benchmark
 
-        seen_key = (section, data[0]) if section == "group" else section
-        if seen_key in seen:
-            # For now each section_key can only show up once.
-            raise NotImplementedError((seen_key, data))
-        seen.add(seen_key)
-
-        if section == 'includes':
-            pass
-        elif section == 'benchmarks':
-            entries = ((s, m, filename) for s, m in data)
-            self._add_benchmarks(entries, resolve)
-        elif section == 'groups':
-            for name in data:
-                self._add_group(name, None)
-        elif section == 'group':
+        if section == 'group':
             name, entries = data
             self._add_group(name, entries)
         else:
-            raise NotImplementedError((section, data))
+            # All sections with an identifier have already been handled.
+            if section in seen:
+                # For now each section_key can only show up once.
+                raise NotImplementedError((section, data))
+            seen.add(section)
+
+            if section == 'includes':
+                pass
+            elif section == 'benchmarks':
+                entries = ((s, m, filename) for s, m in data)
+                self._add_benchmarks(entries, resolve)
+            elif section == 'groups':
+                for name in data:
+                    self._add_group(name, None)
+            else:
+                raise NotImplementedError((section, data))
 
     def _add_benchmarks(self, entries, resolve):
         for spec, metafile, filename in entries:
@@ -145,6 +148,8 @@ def _add_benchmarks(self, entries, resolve):
     def _add_benchmark(self, spec, metafile, resolve, filename):
         if spec.name in self._raw_groups:
             raise ValueError(f'a group and a benchmark have the same name ({spec.name})')
+        if spec.name == 'all':
+            raise ValueError('a benchmark named "all" is not allowed ("all" is reserved for selecting the full set of declared benchmarks)')
         if metafile:
             if filename:
                 localdir = os.path.dirname(filename)
@@ -156,6 +161,8 @@ def _add_benchmark(self, spec, metafile, resolve, filename):
         self._raw_benchmarks.append((spec, metafile, filename))
         if resolve is not None:
             bench = resolve(bench)
+        if bench.name in self._byname:
+            raise ValueError(f'a benchmark named {bench.name} was already declared')
         self._byname[bench.name] = bench
         self._groups = None  # Force re-resolution.
         self._tags = None  # Force re-resolution.
@@ -164,18 +171,15 @@ def _add_group(self, name, entries):
         if name in self._byname:
             raise ValueError(f'a group and a benchmark have the same name ({name})')
         if name == 'all':
-            # XXX Emit a warning?
-            return
-        if entries:
-            raw = self._raw_groups.get(name)
-            if raw is None:
-                raw = self._raw_groups[name] = list(entries) if entries else None
-            elif entries is not None:
-                raw.extend(entries)
-        elif name in self._raw_groups:
-            return
-        else:
+            raise ValueError('a group named "all" is not allowed ("all" is reserved for selecting the full set of declared benchmarks)')
+        if entries is None:
+            if name in self._raw_groups:
+                return
             self._raw_groups[name] = None
+        elif name in self._raw_groups and self._raw_groups[name] is not None:
+            raise ValueError(f'a group named {name} was already defined')
+        else:
+            self._raw_groups[name] = list(entries) if entries else []
         self._groups = None  # Force re-resolution.
 
     def _custom_groups(self):
@@ -218,7 +222,7 @@ def resolve_group(self, name, *, fail=True):
             groups = self._resolve_groups()
             benchmarks = groups.get(name)
             if not benchmarks:
-                if name in (set(self._raw_groups) - {'default'}):
+                if name not in self._raw_groups:
                     benchmarks = self._get_tags().get(name, ())
                 elif fail:
                     raise KeyError(name)

pyperformance/cli.py

@@ -108,6 +108,9 @@ def parse_args():
         'list_groups', help='List benchmark groups of the running Python')
     cmds.append(cmd)
     cmd.add_argument("--manifest", help="benchmark manifest file to use")
+    cmd.add_argument("--tags", action="store_true")
+    cmd.add_argument("--no-tags", dest="tags", action="store_false")
+    cmd.set_defaults(tags=True)
 
     # compile
     cmd = subparsers.add_parser(
@@ -302,7 +305,7 @@ def _main():
         cmd_list(options, benchmarks)
     elif options.action == 'list_groups':
         manifest = _manifest_from_options(options)
-        cmd_list_groups(manifest)
+        cmd_list_groups(manifest, showtags=options.tags)
     else:
         parser.print_help()
         sys.exit(1)

pyperformance/commands.py

@@ -11,7 +11,7 @@ def cmd_list(options, benchmarks):
     print("Total: %s benchmarks" % len(benchmarks))
 
 
-def cmd_list_groups(manifest):
+def cmd_list_groups(manifest, *, showtags=True):
     all_benchmarks = set(manifest.benchmarks)
 
     groups = sorted(manifest.groups - {'all', 'default'})
@@ -28,6 +28,27 @@ def cmd_list_groups(manifest):
             print("- %s" % spec.name)
         print()
 
+    if showtags:
+        print("=============================")
+        print()
+        print("tags:")
+        print()
+        tags = sorted(manifest.tags or ())
+        if not tags:
+            print("(no tags)")
+        else:
+            for tag in tags:
+                specs = list(manifest.resolve_group(tag))
+                known = set(specs) & all_benchmarks
+                if not known:
+                    # skip empty groups
+                    continue
+
+                print("%s (%s):" % (tag, len(specs)))
+                for spec in sorted(specs):
+                    print("- %s" % spec.name)
+                print()
+
 
 def cmd_venv_create(options, root, python, benchmarks):
     from . import _pythoninfo, _venv