Merge pull request #41 from nicholasjng/staging

Add generalized benchmark IO, refactor console reporting, harmonize `pybm env` interface

Author: nicholasjng

.pre-commit-config.yaml

@@ -11,7 +11,7 @@ repos:
     files: pybm/
 
 - repo: https://github.com/psf/black
-  rev: 21.12b0
+  rev: 22.1.0
   hooks:
     - id: black
       language_version: python3

pybm/builders/base.py

@@ -52,7 +52,7 @@ def list(self, executable: Union[str, Path], verbose: bool = False) -> List[str]
     def uninstall(
         self,
         spec: PythonSpec,
-        packages: List[str],
+        packages: Optional[List[str]] = None,
         requirements_file: Optional[str] = None,
         options: Optional[List[str]] = None,
         verbose: bool = False,

pybm/builders/stdlib.py

@@ -74,7 +74,7 @@ class VenvBuilder(BaseBuilder):
 
     def __init__(self, config: PybmConfig):
         super().__init__(config=config)
-        self.venv_home: str = config.get_value("builder.homedir")
+        self.venv_home: Path = Path(config.get_value("builder.homedir"))
 
         # persistent venv options
         self.venv_options: List[str] = []
@@ -254,11 +254,12 @@ def install(
         spec.update_packages(packages=new_packages)
 
     def link(self, env_dir: Union[str, Path], verbose: bool = False):
-        # TODO: This discovery stuff should go into caller routine
-        if (Path(self.venv_home) / env_dir).exists():
-            path = Path(self.venv_home) / env_dir
+        home_path = self.venv_home / env_dir
+        if home_path.exists():
+            path = home_path
         else:
             path = Path(env_dir)
+
         if not builder_util.is_valid_venv(path, verbose=verbose):
             msg = (
                 f"The specified path {str(env_dir)} was not recognized as a valid "
@@ -293,7 +294,7 @@ def list(self, executable: Union[str, Path], verbose: bool = False) -> List[str]
     def uninstall(
         self,
         spec: PythonSpec,
-        packages: List[str],
+        packages: Optional[List[str]] = None,
         requirements_file: Optional[str] = None,
         pip_options: Optional[List[str]] = None,
         verbose: bool = False,
@@ -304,7 +305,12 @@ def uninstall(
         executable = spec.executable
 
         # do not ask for confirmation
-        command = [executable, "-m", "pip", "uninstall", "-y", *packages]
+        command = [executable, "-m", "pip", "uninstall", "-y"]
+        if packages is not None:
+            command += packages
+        elif requirements_file is not None:
+            command += ["-r", requirements_file]
+
         command += list(set(options))
 
         with pip_context("uninstall", spec.root, packages, None):

pybm/builders/util.py

@@ -5,7 +5,7 @@
 from pybm.exceptions import BuilderError
 from pybm.specs import PythonSpec
 from pybm.util.common import version_tuple
-from pybm.util.path import get_subdirs, list_contents
+from pybm.util.path import get_subdirs, get_filenames
 from pybm.util.subprocess import run_subprocess
 
 
@@ -31,6 +31,7 @@ def get_executable(root: Union[str, Path]) -> str:
 
 def is_valid_venv(path: Union[str, Path], verbose: bool = False) -> bool:
     """Check if a directory is a valid virtual environment."""
+
     subdir_set = set(get_subdirs(path=path))
     if os.name != "nt":
         exec_set, bin_folder = {"pip", "python"}, "bin"
@@ -69,7 +70,7 @@ def is_valid_venv(path: Union[str, Path], verbose: bool = False) -> bool:
             end="",
         )
 
-    actual_set = set(list_contents(bin_dir, names_only=True))
+    actual_set = set(get_filenames(bin_dir))
 
     if not exec_set <= actual_set:
         if verbose:

pybm/commands/compare.py

@@ -5,7 +5,6 @@
 from pybm.config import get_component_class
 from pybm.reporters import BaseReporter
 from pybm.status_codes import ERROR, SUCCESS
-from pybm.util.path import get_subdirs
 
 
 class CompareCommand(CLICommand):
@@ -73,16 +72,13 @@ def run(self, args: List[str]) -> int:
         reporter: BaseReporter = get_component_class("reporter", config=self.config)
 
         refs: List[str] = options.refs
-        n_previous: int = options.include_previous
-        report_absolutes: bool = options.absolute
-
-        result_dir = reporter.result_dir
-        results = sorted(get_subdirs(result_dir), key=int)[-n_previous:]
+        previous: int = options.include_previous
+        absolute: bool = options.absolute
 
         reporter.compare(
             *refs,
-            results=results,
-            report_absolutes=report_absolutes,
+            absolute=absolute,
+            previous=previous,
             target_filter=options.target_filter,
             benchmark_filter=options.benchmark_filter,
             context_filter=options.context_filter,

pybm/commands/env.py

@@ -1,12 +1,10 @@
 import argparse
-from contextlib import ExitStack
 from typing import List, Callable, Mapping, Optional
 
 from pybm.builders import BaseBuilder
 from pybm.command import CLICommand
 from pybm.config import PybmConfig, get_component_class
 from pybm.env_store import EnvironmentStore
-from pybm.exceptions import PybmError
 from pybm.logging import get_logger
 from pybm.status_codes import ERROR, SUCCESS
 
@@ -27,7 +25,7 @@ class EnvCommand(CLICommand):
         "   or: pybm env install <identifier> <packages> [<options>]\n"
         "   or: pybm env uninstall <identifier> <packages> [<options>]\n"
         "   or: pybm env list\n"
-        "   or: pybm env update <env> <attr> <value>\n"
+        "   or: pybm env switch <env> <ref>\n"
     )
 
     def __init__(self):
@@ -93,12 +91,12 @@ def add_arguments(self, subcommand: str = None):
                 "environment. Raises an error if the path does not exist or is not "
                 "recognized as a valid Python virtual environment.",
             )
-        elif subcommand in ["delete", "install", "uninstall"]:
+        elif subcommand == "delete":
             self.parser.add_argument(
                 "identifier",
                 metavar="<id>",
-                help="Information that uniquely identifies the environment. "
-                "Can be name, checked out commit/branch/tag, or worktree directory.",
+                help="Information that uniquely identifies the environment. Can be "
+                "name, checked out commit/branch/tag, or worktree directory.",
             )
             if subcommand == "delete":
                 self.parser.add_argument(
@@ -107,19 +105,35 @@ def add_arguments(self, subcommand: str = None):
                     action="store_true",
                     help="Force worktree removal, including untracked files.",
                 )
-            else:
-                self.parser.add_argument(
-                    "packages",
-                    nargs="*",
-                    default=None,
-                    metavar="<packages>",
-                    help="Package dependencies to install into the new virtual "
-                    "environment.",
-                )
+        elif subcommand in ["install", "uninstall"]:
+            self.parser.add_argument(
+                "name",
+                metavar="<name>",
+                help="Information that uniquely identifies the environment. Can be "
+                "name, checked out (partial) commit/branch/tag, or worktree directory.",
+            )
+            self.parser.add_argument(
+                "packages",
+                nargs="*",
+                default=None,
+                metavar="<packages>",
+                help="Package dependencies to install into / uninstall from the new "
+                "virtual environment.",
+            )
         elif subcommand == "list":
             pass
-        elif subcommand == "update":
-            pass
+        elif subcommand == "switch":
+            self.parser.add_argument(
+                "name",
+                metavar="<name>",
+                help="Name of the benchmark environment to switch checkout for.",
+            )
+            self.parser.add_argument(
+                "ref",
+                metavar="<ref>",
+                help="New git reference to check out in the chosen environment. Can "
+                "be a branch name, tag, or (partial) commit SHA.",
+            )
 
         assert subcommand is not None, "no valid subcommand specified"
 
@@ -134,7 +148,7 @@ def add_arguments(self, subcommand: str = None):
             )
             builder_group = self.parser.add_argument_group(builder_group_desc)
 
-            # add builder-specific options into the group
+            # add builder-specific options
             for arg in builder_args:
                 builder_group.add_argument(arg.pop("flags"), **arg)
 
@@ -191,53 +205,33 @@ def install(self, options: argparse.Namespace):
         verbose: bool = option_dict.pop("verbose")
 
         # env name / git worktree info
-        identifier: str = option_dict.pop("identifier")
+        info: str = option_dict.pop("identifier")
 
-        # builder arguments
         packages: Optional[List[str]] = option_dict.pop("packages")
 
-        builder: BaseBuilder = get_component_class("builder", config=self.config)
-
         env_store = EnvironmentStore(config=self.config, verbose=verbose)
 
-        target_env = env_store.get(identifier)
-
-        with ExitStack() as ctx:
-            ctx.callback(env_store.save)
-            builder.install(
-                spec=target_env.python,
-                packages=packages,
-                verbose=verbose,
-                **option_dict,
-            )
+        env_store.install(info=info, packages=packages, verbose=verbose, **option_dict)
 
         return SUCCESS
 
     def uninstall(self, options: argparse.Namespace):
         option_dict = vars(options)
 
-        # verbosity
         verbose: bool = option_dict.pop("verbose")
 
-        identifier: str = option_dict.pop("identifier")
+        info: str = option_dict.pop("identifier")
 
-        # builder arguments
         packages: List[str] = option_dict.pop("packages")
 
-        builder: BaseBuilder = get_component_class("builder", config=self.config)
-
         env_store = EnvironmentStore(config=self.config, verbose=verbose)
 
-        target_env = env_store.get(identifier)
-
-        with ExitStack() as ctx:
-            ctx.callback(env_store.save)
-            builder.uninstall(
-                spec=target_env.python,
-                packages=packages,
-                verbose=verbose,
-                **option_dict,
-            )
+        env_store.uninstall(
+            info=info,
+            packages=packages,
+            verbose=verbose,
+            **option_dict,
+        )
 
         return SUCCESS
 
@@ -249,8 +243,13 @@ def list(self, options: argparse.Namespace):
 
         return SUCCESS
 
-    def update(self, options: argparse.Namespace):
-        raise PybmError("env updating is not implemented yet.")
+    def switch(self, options: argparse.Namespace):
+        name: str = options.name
+        ref: str = options.ref
+        verbose: bool = options.verbose
+
+        env_store = EnvironmentStore(config=self.config, verbose=verbose)
+        env_store.switch(name=name, ref=ref)
 
     def run(self, args: List[str]):
         logger.debug(f"Running command: `{self.format_call(args)}`")
@@ -261,7 +260,7 @@ def run(self, args: List[str]):
             "install": self.install,
             "uninstall": self.uninstall,
             "list": self.list,
-            "update": self.update,
+            "switch": self.switch,
         }
 
         if not args or args[0] not in subcommand_handlers:

pybm/commands/init.py

@@ -7,7 +7,7 @@
 from pybm.exceptions import PybmError
 from pybm.status_codes import SUCCESS
 from pybm.util.git import is_main_worktree
-from pybm.util.path import list_contents
+from pybm.util.path import get_filenames
 
 
 class InitCommand(CLICommand):
@@ -93,7 +93,7 @@ def run(self, args: List[str]) -> int:
             config_dir.mkdir(parents=False, exist_ok=True)
 
         if options.remove_existing:
-            for p in list_contents(config_dir, file_suffix=".toml", names_only=True):
+            for p in get_filenames(config_dir, file_ext=".toml"):
                 (config_dir / p).unlink()
 
         if Path(LOCAL_CONFIG).exists():

pybm/commands/run.py

@@ -1,4 +1,3 @@
-import json
 import warnings
 from pathlib import Path
 from typing import List, Optional
@@ -7,8 +6,9 @@
 from pybm.config import PybmConfig, get_component_class
 from pybm.env_store import EnvironmentStore
 from pybm.exceptions import PybmError
+from pybm.reporters import BaseReporter
 from pybm.runners import BaseRunner
-from pybm.runners.util import create_subdir, create_rundir, discover_targets
+from pybm.runners.util import discover_targets
 from pybm.status_codes import SUCCESS, ERROR
 
 
@@ -126,6 +126,7 @@ def run(self, args: List[str]) -> int:
         runner_options = vars(options)
 
         runner: BaseRunner = get_component_class("runner", config=self.config)
+        reporter: BaseReporter = get_component_class("reporter", config=self.config)
 
         verbose: bool = runner_options.pop("verbose")
 
@@ -142,8 +143,6 @@ def run(self, args: List[str]) -> int:
         benchmark_context = runner_options.pop("benchmark_context")
         # at this point, runner_options only include the additional runner kwargs
 
-        result_dir = create_rundir(runner.result_dir)
-
         if source_path.is_absolute():
             raise PybmError(
                 f"Benchmark path {source_path!r} was given in absolute form. Please "
@@ -193,8 +192,6 @@ def run(self, args: List[str]) -> int:
             worktree = environment.worktree
             ref, ref_type = worktree.get_ref_and_type()
 
-            subdir = create_subdir(result_dir=result_dir, worktree=worktree)
-
             runner.check_required_packages(environment=environment)
 
             with discover_targets(
@@ -241,23 +238,17 @@ def run(self, args: List[str]) -> int:
 
                     if rc != 0:
                         raise PybmError(
-                            "Something went wrong during the benchmark. stderr output "
+                            "Something went wrong during the benchmark. Stderr output "
                             f"of the dispatched subprocess:\n{data}"
                         )
                     elif not data:
                         raise PybmError(
                             "No result data was obtained from the dispatched "
                             "benchmark subprocess. Please check that the configured "
-                            "benchmark runner actually writes the results to stdout. "
-                            "If you are using the Google Benchmark runner, please "
-                            "adapt your benchmark files to use Google Benchmark."
+                            "benchmark runner actually writes the results to stdout."
                         )
                     else:
-                        # TODO: Switch this to a general IO Connector
-                        result_name = Path(benchmark).stem + "_results.json"
-                        result_file = subdir / result_name
-                        with open(result_file, "w") as res:
-                            json.dump(json.loads(data), res)
+                        reporter.write(ref, benchmark, data)
 
         if checkout_mode:
             root_ref, root_type = root_checkout

pybm/config.py

@@ -205,5 +205,7 @@ def get_runner_requirements(config: PybmConfig) -> List[str]:
         "ms/msec, us/usec, and ns/nsec, where either spelling is admissible.",
         "significantdigits": "Number of significant digits to round floating point "
         "results to in console display.",
+        "shalength": "Length of git SHA fragments to display in console output. "
+        "Default value is 8, meaning the first eight hex digits are displayed.",
     },
 }