voltrondata-labs / arrowbench

R package for benchmarking
Other
13 stars 9 forks source link

Use benchconnect to publish results to a Conbench server #127

Closed alistaire47 closed 1 year ago

alistaire47 commented 1 year ago

This PR:

  1. integrates benchconnect with the run.BenchmarkDataFrame() S3 method when publish = TRUE to allow arrowbench to publish directly to a Conbench server instead of requiring wrapping via voltrondata-labs/benchmarks.
  2. adapts the management of the external benchconnect dependency from #113, leaving the bare bones of datalogistik installation in place as well, though obviously not the full integration that PR entails.
  3. adds {processx} as a dependency, as it allows finer control than system(). In the future we may consider moving some of the preexisting system() calls to processx::run() as well if they get troublesome.
  4. aligns with https://github.com/conbench/conbench/pull/587 in its use of environment variables for passing things like github metadata.
  5. populates errorred benchmarks sufficiently to publish them to Conbench, closing #121. It does not populate validation or the run-level error fields yet, but it will at least let us see what's failing provided the whole run does not fail.
  6. Unit tests call benchconnect augment, but mock calls that require an active Conbench server (benchconnect start run, benchconnect submit result, and benchconnect finish run). https://github.com/voltrondata-labs/arrow-benchmarks-ci/issues/94 would integration test the full toolchain; for now I've run it against a local server instance successfully.

This publishing will not be active in real benchmark running until arrow-benchmarks-ci is rearranged to call it.

alistaire47 commented 1 year ago

@austin3dickey I can't add you as a reviewer, but please take a look anyway if you've got time

alistaire47 commented 1 year ago

I did try out the installation process and after some paths futzing it did work. The installation proceeded super smoothly. Nice. Paths promise to be a challenge but if there are only a few users here then smoothing that out might not be worth the effort.

In theory, we only need benchconnect available on machines from which we publish benchmarks, so provided we can get it installed in CI, we're pretty much ok. But datalogistik will have to be installed everywhere we run benchmarks (including locally), so ultimately we do need this system to work well enough that benchmark authors have a good experience. That's probably never going to include an enormous number of people, so I don't think that means CRAN standards, necessarily, but we do want it to be a good experience regardless.

One possible TODO from this (and is probably something that could happen when you tackle the CI stuff) is a write up how one would do this from benchmark to posting. In fact it might be useful for someone like me who is less close to this to actually write that document and then you can tell me all the ways that I am wrong. If that sounds useful, let me know when you take on the CI part of this and we can collaborate.

Like the process of writing a new benchmark and getting it running via CI? That sounds useful, though yeah, it's dependent on my next CI story. We could use it to revamp the README a bit (it's got a lot of useful stuff there, but could use some curation to align with user stories), maybe? Or turn it into a vignette if we don't want to touch that.

boshek commented 1 year ago

it's dependent on my next CI story

When you get there let me know. I think this is probably a useful exercise.

alistaire47 commented 1 year ago

Ok, this PR should be pretty much good to merge now, if y'all could take a quick look through the changes since your last reviews 🙏

jonkeane commented 1 year ago

Let's merge this and get it into use. We're at the point that we need to shake out issues by actually using this — we might come back and refine more that's 100% ok and what we should do