-
Notifications
You must be signed in to change notification settings - Fork 9
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
Iterative benchadapt adapter thats calls arrowbench benchmarks one at a time #115
base: main
Are you sure you want to change the base?
Iterative benchadapt adapter thats calls arrowbench benchmarks one at a time #115
Conversation
… one case at a time
Failing test is unrelated; something about duckdb + tpch: ══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-custom-duckdb.R:26'): custom DuckDB can be installed to and used from a custom lib ──
Error in `ensure_custom_duckdb(tf, install = FALSE)`: An unexpected error occured whilst querying TPC-H enabled duckdb
Caused by error:
! error in callr subprocess
Caused by error:
! rapi_prepare: Failed to prepare query select scale_factor, query_nr from tpch_answers() LIMIT 1;
Error: Error: Catalog Error: Function with name tpch_answers is not on the catalog, but it exists in the tpch extension. To Install and Load the extension, run: INSTALL tpch; LOAD tpch;
Backtrace:
▆
1. ├─testthat::expect_error(...) at test-custom-duckdb.R:26:2
2. │ └─testthat:::expect_condition_matching(...)
3. │ └─testthat:::quasi_capture(...)
4. │ ├─testthat (local) .capture(...)
5. │ │ └─base::withCallingHandlers(...)
6. │ └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
7. └─arrowbench:::ensure_custom_duckdb(tf, install = FALSE)
8. └─base::tryCatch(...)
9. └─base (local) tryCatchList(expr, classes, parentenv, handlers)
10. └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
11. └─value[[3L]](cond)
12. └─rlang::abort(...) Probably this will go away with the switch to datalogistik? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what inst/arrowbench-adapter.py
is for with the framework you're using — but that it needs to exist at all in this repository is a sign that we don't have the setup here well-factored and we aren't appropriately separating concerns of the adapter and the runner (since we have to commingle code like this in order to use the adapter in places like {arrowbench}). Which comes down to what I describe at: conbench/conbench#406 (review) I've also put more of this design in conbench/conbench#368 for the record + so we can come back to that and refine it if we need to.
It's not a great implementer-UX that in order to take advantage of the adapters code we are building, one has to know enough of the internals of the adapter + know enough Python to write out a file like inst/arrowbench-adapter.py
and pass back and forth the specifications for what is being run. I do truly believe that this approach is going to cause us more harm than good — and would like to see that approach I laid out above where the adapter is totally separate and callable iteratively taken seriously + you at least attempt an implementation of it before writing it off. And it's going to require more hand-holding, needing to edit other people's code in order for them to be able to use this, and more convincing by us to roll out than an adapter that is more encapsulated.
All of that being said, if you believe that the co-mingled approach in this PR (+ in conbench/conbench#406 and conbench/conbench#415, though the pytest one doesn't yet work iteratively — I believe you mentioned that already, but I don't see that comment in the PR so I might be misremembering!), let's go with that + work on getting this rolled out to all of the places it needs to be (arrowbench, the go benchmarks, the arrow python benchmarks, as well as the various internal benchmarks set ups we have — and anywhere else that I'm missing here). Let's document clearly what the steps are to do things like these and also document clearly why we made the decisions we did with this setup.
Mocks up an iterative benchadapt adapter using
GeneratorAdapter
from conbench/conbench#406 to call arrowbench benchmarks one at a time. Changes:BenchmarkResult
to allow it to output JSON with fields arranged appropriately forbenchadapt.BenchmarkResult
(and therefore the conbench API). A little fast-and-dirty; there's work to be done to make sure we're populating everything what we want to, since all of this was getting populated by voltrondata_labs/benchmarks and the conbench runner before.read_file
andwrite_file
to match the defaults in labs/benchmarks.names(known_sources)
was working fine except fortpch
, for which the benchmarks don't have additional code to iterate over tables.inst/arrowbench
that can:a. produce a JSON list of dicts of args suitable for running a case via
run_one()
, e.g.{"bm": "read_file", "source": "fanniemae_2016Q4", "format": "parquet", "compression": "uncompressed", "output": "arrow_table", "cpu_count": 1}
b. run a case with
run_one()
when passed a dict of args like the list command produces and cat the cleaned JSON resultsGeneratorAdapter
. It is initialized with a path to the CLI created in 3., and when run, hits the CLI to list benchmark cases, then iterates through them (subset to the first 10 for demo purposes), runs each, parses the result JSON and inserts it into aBenchmarkResult
object, and yields that. When called, the adapter will post each result before moving on to the next iteration.This needs careful polish before replacing labs/benchmarks for R benchmark running to ensure that our metadata is consistent and won't break histories, but does show how R benchmarks could work with adapters.