Address reviews

Author: AntoinePrv

dev/archery/archery/benchmark/runner.py

@@ -20,6 +20,7 @@
 import os
 import random
 import re
+import subprocess
 
 from .core import BenchmarkSuite
 from .google import GoogleBenchmarkCommand, GoogleBenchmark
@@ -34,14 +35,20 @@
 
 
 def _rev_or_path_dirname(src, rev_or_path):
+    """Return a directory-name-safe identifier for a revision or a path.
+
+    If ``rev_or_path`` resolves to a git revision, its full SHA is returned.
+    Otherwise (e.g. it is a filesystem path), a sanitized form with path
+    separators replaced is returned.
+    """
     if rev_or_path == ArrowSources.WORKSPACE:
         return rev_or_path
     try:
         sha = git.rev_parse(rev_or_path, git_dir=src.path)
         if isinstance(sha, bytes):
             sha = sha.decode("ascii")
         return sha
-    except Exception:
+    except subprocess.CalledProcessError:
         return rev_or_path.replace("/", "_")
 
 
@@ -56,8 +63,13 @@ def regex_filter(re_expr):
 
 
 class BenchmarkRunner:
-    def __init__(self, suite_filter=None, benchmark_filter=None,
-                 repetitions=DEFAULT_REPETITIONS, repetition_min_time=None):
+    def __init__(
+        self,
+        suite_filter=None,
+        benchmark_filter=None,
+        repetitions=DEFAULT_REPETITIONS,
+        repetition_min_time=None,
+    ):
         self.suite_filter = suite_filter
         self.benchmark_filter = benchmark_filter
         self.repetitions = repetitions
@@ -70,12 +82,11 @@ def suites(self):
 
     @staticmethod
     def from_rev_or_path(src, root, rev_or_path, cmake_conf, **kwargs):
-        raise NotImplementedError(
-            "BenchmarkRunner must implement from_rev_or_path")
+        raise NotImplementedError("BenchmarkRunner must implement from_rev_or_path")
 
 
 class StaticBenchmarkRunner(BenchmarkRunner):
-    """ Run suites from a (static) set of suites. """
+    """Run suites from a (static) set of suites."""
 
     def __init__(self, suites, **kwargs):
         self._suites = suites
@@ -110,6 +121,7 @@ def is_json_result(cls, path_or_str):
     def from_json(path_or_str, **kwargs):
         # .codec imported here to break recursive imports
         from .codec import BenchmarkRunnerCodec
+
         if os.path.isfile(path_or_str):
             with open(path_or_str) as f:
                 loaded = json.load(f)
@@ -122,11 +134,12 @@ def __repr__(self):
 
 
 class CppBenchmarkRunner(BenchmarkRunner):
-    """ Run suites from a CMakeBuild. """
+    """Run suites from a CMakeBuild."""
 
-    def __init__(self, build, benchmark_extras, run_id=None,
-                 results_dir=None, **kwargs):
-        """ Initialize a CppBenchmarkRunner. """
+    def __init__(
+        self, build, benchmark_extras, run_id=None, results_dir=None, **kwargs
+    ):
+        """Initialize a CppBenchmarkRunner."""
         super().__init__(**kwargs)
         self.build = build
         self.benchmark_extras = benchmark_extras
@@ -135,9 +148,11 @@ def __init__(self, build, benchmark_extras, run_id=None,
 
     @staticmethod
     def default_configuration(**kwargs):
-        """ Returns the default benchmark configuration. """
+        """Returns the default benchmark configuration."""
         return CppConfiguration(
-            build_type="release", with_tests=False, with_benchmarks=True,
+            build_type="release",
+            with_tests=False,
+            with_benchmarks=True,
             with_compute=True,
             with_csv=True,
             with_dataset=True,
@@ -152,30 +167,32 @@ def default_configuration(**kwargs):
             with_snappy=True,
             with_zlib=True,
             with_zstd=True,
-            **kwargs)
+            **kwargs,
+        )
 
     @property
     def suites_binaries(self):
-        """ Returns a list of benchmark binaries for this build. """
+        """Returns a list of benchmark binaries for this build."""
         # Ensure build is up-to-date to run benchmarks
         self.build()
         # Not the best method, but works for now
         glob_expr = os.path.join(self.build.binaries_dir, "*-benchmark")
         return {os.path.basename(b): b for b in glob.glob(glob_expr)}
 
     def suite(self, name, suite_bin):
-        """ Returns the resulting benchmarks for a given suite. """
-        suite_cmd = GoogleBenchmarkCommand(suite_bin, self.benchmark_filter,
-                                           self.benchmark_extras)
+        """Returns the resulting benchmarks for a given suite."""
+        suite_cmd = GoogleBenchmarkCommand(
+            suite_bin, self.benchmark_filter, self.benchmark_extras
+        )
 
         # Ensure there will be data
         benchmark_names = suite_cmd.list_benchmarks()
         if not benchmark_names:
             return None
 
         results = suite_cmd.results(
-            repetitions=self.repetitions,
-            repetition_min_time=self.repetition_min_time)
+            repetitions=self.repetitions, repetition_min_time=self.repetition_min_time
+        )
         benchmarks = GoogleBenchmark.from_json(results.get("benchmarks"))
         return BenchmarkSuite(name, benchmarks)
 
@@ -188,7 +205,7 @@ def list_benchmarks(self):
 
     @property
     def suites(self):
-        """ Returns all suite for a runner. """
+        """Returns all suite for a runner."""
         suite_matcher = regex_filter(self.suite_filter)
 
         suite_found = False
@@ -214,7 +231,7 @@ def suites(self):
 
     @staticmethod
     def from_rev_or_path(src, root, rev_or_path, cmake_conf, **kwargs):
-        """ Returns a BenchmarkRunner from a path or a git revision.
+        """Returns a BenchmarkRunner from a path or a git revision.
 
         First, it checks if `rev_or_path` is a valid path (or string) of a json
         object that can deserialize to a BenchmarkRunner. If so, it initialize
@@ -230,7 +247,7 @@ def from_rev_or_path(src, root, rev_or_path, cmake_conf, **kwargs):
         """
         build = None
         if StaticBenchmarkRunner.is_json_result(rev_or_path):
-            kwargs.pop('benchmark_extras', None)
+            kwargs.pop("benchmark_extras", None)
             return StaticBenchmarkRunner.from_json(rev_or_path, **kwargs)
         elif CMakeBuild.is_build_dir(rev_or_path):
             build = CMakeBuild.from_path(rev_or_path)
@@ -240,6 +257,8 @@ def from_rev_or_path(src, root, rev_or_path, cmake_conf, **kwargs):
             root_rev = os.path.join(root, path_rev)
             os.makedirs(root_rev, exist_ok=True)
 
+            # Clone dir is reused when there is known revision (git sha)
+            # in <preserve-dir>/sha/arrow
             clone_dir = os.path.join(root_rev, "arrow")
             if os.path.isdir(clone_dir):
                 src_rev = ArrowSources(clone_dir)
@@ -253,31 +272,31 @@ def from_rev_or_path(src, root, rev_or_path, cmake_conf, **kwargs):
             build = cmake_def.build(build_dir)
             results_dir = os.path.join(root_rev, "bench", "run", run_id)
             os.makedirs(results_dir, exist_ok=True)
-            return CppBenchmarkRunner(build, run_id=run_id,
-                                      results_dir=results_dir, **kwargs)
+            return CppBenchmarkRunner(
+                build, run_id=run_id, results_dir=results_dir, **kwargs
+            )
 
 
 class JavaBenchmarkRunner(BenchmarkRunner):
-    """ Run suites for Java. """
+    """Run suites for Java."""
 
     # default repetitions is 5 for Java microbenchmark harness
     def __init__(self, build, **kwargs):
-        """ Initialize a JavaBenchmarkRunner. """
+        """Initialize a JavaBenchmarkRunner."""
         self.build = build
         super().__init__(**kwargs)
 
     @staticmethod
     def default_configuration(**kwargs):
-        """ Returns the default benchmark configuration. """
+        """Returns the default benchmark configuration."""
         return JavaConfiguration(**kwargs)
 
     def suite(self, name):
-        """ Returns the resulting benchmarks for a given suite. """
+        """Returns the resulting benchmarks for a given suite."""
         # update .m2 directory, which installs target jars
         self.build.build()
 
-        suite_cmd = JavaMicrobenchmarkHarnessCommand(
-            self.build, self.benchmark_filter)
+        suite_cmd = JavaMicrobenchmarkHarnessCommand(self.build, self.benchmark_filter)
 
         # Ensure there will be data
         benchmark_names = suite_cmd.list_benchmarks()
@@ -291,7 +310,7 @@ def suite(self, name):
 
     @property
     def list_benchmarks(self):
-        """ Returns all suite names """
+        """Returns all suite names"""
         # Ensure build is up-to-date to run benchmarks
         self.build.build()
 
@@ -302,7 +321,7 @@ def list_benchmarks(self):
 
     @property
     def suites(self):
-        """ Returns all suite for a runner. """
+        """Returns all suite for a runner."""
         suite_name = "JavaBenchmark"
         suite = self.suite(suite_name)
 
@@ -315,7 +334,7 @@ def suites(self):
 
     @staticmethod
     def from_rev_or_path(src, root, rev_or_path, maven_conf, **kwargs):
-        """ Returns a BenchmarkRunner from a path or a git revision.
+        """Returns a BenchmarkRunner from a path or a git revision.
 
         First, it checks if `rev_or_path` is a valid path (or string) of a json
         object that can deserialize to a BenchmarkRunner. If so, it initialize