Repository navigation
Add a global benchx.conf.json config file for project-wide settings - #65
Conversation
|
The diff is still ~1KLOC. Not all of it is equally important though. The crux of the PR is
to
Thus, the key questions I'd ask for a review of, are the whole idea of the global config file, and whether the schema makes sense, as added by this PR. |
|
As discussed in the call, I'd appreciate a review of this PR if somebody has cycles to spare. |
76a8a6f to
96e3499
Compare
|
Could I ask you take a brief look @MaanasArora ? There's quite a bit of implementation details here, which are probably fairly boring plumbing; The main idea is to be able to collect the majority of configuration options into a project-wide config, and add CLI switches on top of it --- I think it improves usability. There are several examples in the PR itself, e.g. https://github.com/benchx-dev/benchx/pull/65/changes#diff-9dbb3fa728a73cf2d94b3caa9d5a4933213ae649d4987c41d31d92acc8a8c96a, and here's one other example with several bells and whistles at https://github.com/benchx-dev/benchx_examples/blob/main/examples/numpy-spin/benchx.conf.json. This PR sits in a bottom of a "stack" (the next one in the stack is #71). |
…tree identity schemas/config/0.1.0 describes benchx.json (the project root's one config file), with recommended pyperf and Google Benchmark settings as examples that the tests check against the schema and the adapters' own rules. snapshot.ignore() leaves the paths benchx writes (the store, the results) out of git and directory tree identity, so a run's output inside a measured tree does not mark it dirty. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
benchx.json replaces .benchx/provider.json, the old .benchx/config.json and the ~/.benchx home. It holds the project, source, provider, and an adapter block (name, suite, quantities, precision), environment variables, timeouts, paths and the comparator's policy. The directory holding it is the root; the store (store.parquet) and the results default to it, and are left out of tree identity. Every value is the command line's, else the file's, else a default; a required one (adapter.name, adapter.suite, the target, compare.profile and rounds) with none is a hard failure naming both. --quantity takes a comma-separated list, one order collects them all, and --no-project makes a run ad hoc. bx config init writes a template for either harness and bx config show says where each value comes from. The old files are not read: finding one is an error. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The design doc, the scope table and the README describe the project file, how a value is picked, and the removal of .benchx/provider.json, the old .benchx/config.json and the ~/.benchx home. archery-local and pyperf-bench get a benchx.json (the latter now runs `bx bench .`), and the demo gets a fixed project root, examples/demo, so its store persists across runs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
With one project config, the store is the project root's store.parquet and each test's root is its own directory. - test_benchmark_source: read the store as Store(path), not Store(). - test_prototype: the library-level store in the unreadable-files test is its own file, so the CLI ingests into the project's store as the test intends instead of finding the document there already. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MaanasArora
left a comment
MaanasArora
left a comment
There was a problem hiding this comment.
Sorry this sat so long, all looks good, just one very nitpicky question inline! I'll just merge as I don't think we need to address it here really. Thanks @ev-br!
| CONFIG_FILE = "benchx.conf.json" | ||
| # Files of the format this one replaced. They are not read; finding one is an error, so | ||
| # no setting silently stops applying. | ||
| LEGACY_FILES = (Path(".benchx") / "provider.json", Path(".benchx") / "config.json") |
There was a problem hiding this comment.
Is this defense needed? I don't mind it but I'm wondering if anyone is using the old schemas?
There was a problem hiding this comment.
Probably not---I was keeping this mainly to minimize friction for your and Marco's PRs. Let me send a separate clean-up PR.
EDIT: in fact, let me hold on the clean-up for now: I'll first move/consolidate examples and prototype/examples, make sure they run, and then clean up.
Moving examples needs other PRs though: context-collectors, uc-04 and parametrizations. Sigh.
|
Thanks for the review @MaanasArora ! |
Currently, various benchmark run parameters are split between the global config (we save something to
$HOME/.benchx), two ad hoc.benchx/config.jsonand.benchx/provider.json, plus a half dozen CLI arguments.Instead, add a project-wide
benchx.conf.jsonto keep project-wide parameters : name, URI, result/storage paths, adapter-specific parameters. This way, there's a central place for things which are unlikely to change per benchmark, and the CLI gets a bit simpler.This PR is on top of https://github.com/rok/benchx/pull/64. To make the diff easier to see, here's a PR on my fork, against the gh-64 branch :rebased.ev-br#5