Add capability to version cases #105

alistaire47 · 2022-06-08T18:36:57Z

Successor to #104; closes #101. This PR inserts versioning for cases into params via a benchmark-configurable function that takes a case (a named list of params) and returns an integer version.

There are a couple important questions here:

Should version be inserted as a param in the JSON (and therefore in conbench)? We're already inserting a lot of stuff into params that's arguably metadata (cpu_count, lib_path, a dataframe of packages); at some point we should move this stuff to a metadata section.
What should the default behavior be? This sets everything to version 1L, but I haven't quite figured out yet whether this will break all the histories in conbench. If so, it's probably better to change it to default to not setting it.

alistaire47 · 2022-06-08T21:21:22Z

Regarding 2, what exactly does create a history discontinuity in conbench? @austin3dickey have you happened to learn this yet?

jonkeane · 2022-06-08T21:36:14Z

Regarding 2, what exactly does create a history discontinuity in conbench? @austin3dickey have you happened to learn this yet?

We should document this since it comes up frequently, but:

Differences in:

case values "tags"
contexts
hardware (though not all values in a hardware are taken into account, the additional info, for example is allowed to vary)

https://github.com/conbench/conbench/blob/a3073041f702a4da8ad708afa776be6d7ab0ca72/conbench/entities/history.py#L35-L68

jonkeane · 2022-06-08T21:40:59Z

What should the default behavior be? This sets everything to version 1L, but I haven't quite figured out yet whether this will break all the histories in conbench. If so, it's probably better to change it to default to not setting it.

We definitely shouldn't break history — we could either migrate and add 1s in everywhere or we could have a special case in conbench history where for version NULL == 1

tests/testthat/test-run.R

jonkeane · 2022-06-08T21:44:25Z

Should version be inserted as a param in the JSON (and therefore in conbench)? We're already inserting a lot of stuff into params that's arguably metadata (cpu_count, lib_path, a dataframe of packages); at some point we should move this stuff to a metadata section.

Hmm, yeah I think this is ok. It is like many of the other variables that defines a case (at least on the conbench side)...

alistaire47 · 2022-06-08T21:44:28Z

Differences in:

* case values "tags"

* contexts

* hardware (though not _all_ values in a hardware are taken into account, the additional info, for example is allowed to vary)

Hm, so we are writing most params (currently including case_version on this branch) to tags. I think the simplest approach is to default to not writing it then (but do write it when it's explicitly set!); otherwise we'll need special-casing elsewhere

…test versioning

alistaire47 · 2022-06-08T22:37:32Z

R/benchmark.R

@@ -93,6 +98,8 @@ Benchmark <- function(name,
                      after_each = TRUE,
                      teardown = TRUE,
                      valid_params = function(params) params,
+                      case_version = function(params) NULL,
+                      packages_used = function(params) "arrow",


Added this because it's already called, but previously we were relying on every benchmark adding it in the same way though .... I'm now leaning more towards R6-ifying this class

boshek

Not a ton to say other than that it feels like until we actually need a version change we should stick with the NULL == 1 situation.

alistaire47 · 2022-06-09T21:38:17Z

Not a ton to say other than that it feels like until we actually need a version change we should stick with the NULL == 1 situation.

Right now NULL != 1, because when it's NULL it won't add case_version to tags, but when it's 1 it will (and therefore will break history). Dunno if that's good, really, but it's easiest, as if we start defaulting everything to 1 it will break all the histories unless we go adjust everything

jonkeane

This is looking good — thanks! A few comments that might be quick to fix or a longer thread to unravel.

jonkeane · 2022-06-14T15:56:55Z

tests/testthat/test-run.R

+  bm_versioned <- Benchmark(
+    "versioned",
+    setup = function(x = c('foo', 'bar')) cat(x),
+    case_version = function(params) c("foo" = 1L, "bar" = 2L)[params$x]


I can and will look at the code, but we should also test what happens if foo is not mentioned here? Is that possible?

Because of how I constructed this example, if params were list(x = "baz") (or any novel value), it will return NA, which will get written to the tags as "tags": {..., "case_version": null}, which I think may break history in conbench (though I'm not 💯).

jonkeane · 2022-06-14T16:00:37Z

R/run.R

@@ -186,6 +186,8 @@ run_bm <- function(bm, ..., n_iter = 1, profiling = FALSE, global_params = list(
  defaults <- lapply(get_default_args(bm$setup), head, 1)
  defaults$cpu_count <- parallel::detectCores()
  params <- modifyList(defaults, list(...))
+  params$case_version <- bm$case_version(params)


Hmm looks like my question in the tests below: I think this would return NA? Would it be possible to wrap this such that if something isn't defined in the function it'll return NULL?

Yep, that's what I'm thinking; we should either do stopifnot(is.na(case_version)) or automatically replace it with NULL (with a warning, probably?). I'm leaning towards erroring—we should see this when adjusting benchmarks (presuming we run them with all arg combinations), and we don't want to accidentally switch back to not versioning without noticing.

Ok, put in a stopifnot() and a test for it

alistaire47 · 2022-06-14T19:57:38Z

ah crap my git branches got conflated! sorry, fixing

Add capability to version cases

91e3e4d

alistaire47 requested review from jonkeane and boshek June 8, 2022 18:36

alistaire47 self-assigned this Jun 8, 2022

jonkeane reviewed Jun 8, 2022

View reviewed changes

tests/testthat/test-run.R Outdated Show resolved Hide resolved

Change default versioning behavior; add default for packages_used; …

c409a48

…test versioning

alistaire47 commented Jun 8, 2022

View reviewed changes

boshek approved these changes Jun 9, 2022

View reviewed changes

jonkeane reviewed Jun 14, 2022

View reviewed changes

Fail when case version is NA

ee625f1

alistaire47 force-pushed the feat/case-version branch from 8658ed2 to ee625f1 Compare June 14, 2022 19:59

R 3.6 ugh

0b7afe8

alistaire47 merged commit 8faca62 into voltrondata-labs:main Jun 15, 2022

alistaire47 deleted the feat/case-version branch June 15, 2022 20:17

This was referenced Jun 15, 2022

Add capability to version cases voltrondata-labs/benchmarks#106

Closed

Add Python case versioning capability voltrondata-labs/benchmarks#109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capability to version cases #105

Add capability to version cases #105

alistaire47 commented Jun 8, 2022

alistaire47 commented Jun 8, 2022

jonkeane commented Jun 8, 2022

jonkeane commented Jun 8, 2022 •

edited

Loading

jonkeane commented Jun 8, 2022

alistaire47 commented Jun 8, 2022

alistaire47 Jun 8, 2022

boshek left a comment

alistaire47 commented Jun 9, 2022

jonkeane left a comment

jonkeane Jun 14, 2022

alistaire47 Jun 14, 2022

jonkeane Jun 14, 2022

alistaire47 Jun 14, 2022

alistaire47 Jun 14, 2022

alistaire47 commented Jun 14, 2022

Add capability to version cases #105

Add capability to version cases #105

Conversation

alistaire47 commented Jun 8, 2022

alistaire47 commented Jun 8, 2022

jonkeane commented Jun 8, 2022

jonkeane commented Jun 8, 2022 • edited Loading

jonkeane commented Jun 8, 2022

alistaire47 commented Jun 8, 2022

alistaire47 Jun 8, 2022

Choose a reason for hiding this comment

boshek left a comment

Choose a reason for hiding this comment

alistaire47 commented Jun 9, 2022

jonkeane left a comment

Choose a reason for hiding this comment

jonkeane Jun 14, 2022

Choose a reason for hiding this comment

alistaire47 Jun 14, 2022

Choose a reason for hiding this comment

jonkeane Jun 14, 2022

Choose a reason for hiding this comment

alistaire47 Jun 14, 2022

Choose a reason for hiding this comment

alistaire47 Jun 14, 2022

Choose a reason for hiding this comment

alistaire47 commented Jun 14, 2022

jonkeane commented Jun 8, 2022 •

edited

Loading