Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink reporter concept #24

Closed
nicholasjng opened this issue Jan 25, 2024 · 3 comments
Closed

Rethink reporter concept #24

nicholasjng opened this issue Jan 25, 2024 · 3 comments
Milestone

Comments

@nicholasjng
Copy link
Collaborator

Right now, our final step (reporting obtained results) is a bit unfinished. We have a AbstractBenchmarkRunner.report() method, which takes all reporters by name, and does not even use the runner's state, so it is (essentially) static.

At the same time, our Reporter class has no state or worthwhile interface - it too gets a report method, which is an awkward copy of the runner's report method.

Maybe we should untangle that and come up with a better abstraction. How about this:

  1. The benchmark runner is also a reporter (and, for that matter, the only reporter) - a reporter is exactly something that has a report() method.
  2. In place of the current BaseReporter, i.e. the argument given in AbstractBenchmarkRunner.report(to=...), we introduce a new class (Sink? DataSource?) that maybe also reads benchmark results from, but at minimum writes to a given sink.
  3. Any supported sink then gives single output of benchmark results in the desired form - database records, plots, records, text summaries, you name it.
  4. We drop support for sequences of str | BaseReporters, and require the user to loop over the sinks they want to use.

Comments / suggestions welcome.

@nicholasjng
Copy link
Collaborator Author

Decision for v0.1.0 (Feb 1 24):

  1. We drop BenchmarkRunner.report() in favor of a BenchmarkReporter class, which gets a .report() method and (TBD) a lower-level interface to give the user control over single records if he needs it.
  2. We establish the BenchmarkReporter as a class with a predefined interface. Similarly to the BenchmarkRunner, we aim to provide a base interface that is flexible enough that most users can use that out of the box without having to explicitly override methods.
    We use existing packages as inspiration, e.g. GBM's BenchmarkReporter (https://github.com/google/benchmark/blob/main/include/benchmark/benchmark.h#L300) or Beam's Sink (https://github.com/apache/beam/blob/master/sdks/python/apache_beam/io/filebasedsink.py).
  3. We give an array of default implementations to serve some very common needs, like file-based reporting, HTTP transfers, or database streaming.
  4. We need to give the ability to simultaneously report multiple results, either from different subsequent runs or from previous records (this requires a sourcing method on the reporter). For this, we need to be able to merge records.

(Tentatively: Broadcast context onto benchmarks, merge lists)

@nicholasjng
Copy link
Collaborator Author

Rethought the concept properly, see above. We therefore drop this from the milestone, and close once the base reporter interface is established.

@maxmynter
Copy link
Collaborator

Closing as the reporter interface was merged in #52

@maxmynter maxmynter added this to the v0.1.0 milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants