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

Replace nnbench.BenchmarkRunner with modular nnbench.collect() and nnbench.run() APIs #193

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

nicholasjng
Copy link
Collaborator

The class had little to no useful state, anyways, and these two standalone APIs play
way better with threading and concurrency, so we opt for functional APIs instead of
a monolithic class.

This is effectively an idiom change - before, we were instantiating as
runner = nnbench.BenchmarkRunner, whereas now, we go the extra mile to
collect in the open (benchmarks = nnbench.collect(path, tags=...)), and
then pass them as the first argument to the "new" nnbench.run() API, which
is just the BenchmarkRunner.run() with the path and tags arguments removed.

This also prevents erroneous caching and persistence of large params to an extent,
since the benchmark list is made explicit.

We had relatively few consumers, namely the main module and the CLI, so the migration was
not as hard as previously thought.

Replaces the previous zero-state BenchmarkRunner with the new "collect-then-run" idiom
in the examples and documentation.

In most of the docs, this is only a syntax change, but some formulations had to be tweaked,
since there is no more single runner resource.


Work towards #189. I already have a local concurrency setup using multiprocessing up and running, but it is very crude, and does not work by just passing the benchmark objects, due to limitations with Python's pickle with respect to serializing functions.

…`nnbench.run()`

The class had little to no useful state, anyways, and these two standalone APIs play
way better with threading and concurrency, so we opt for functional APIs instead of
a monolithic class.

This is effectively an idiom change - before, we were instantiating as
`runner = nnbench.BenchmarkRunner`, whereas now, we go the extra mile to
collect in the open (`benchmarks = nnbench.collect(path, tags=...)`), and
then pass them as the first argument to the "new" `nnbench.run()` API, which
is just the BenchmarkRunner.run() with the path and tags arguments removed.

This also prevents erroneous caching and persistence of large params to an extent,
since the benchmark list is made explicit.
…diom

We had relatively few consumers, namely the main module and the CLI, so this was
not as hard as previously thought.
This replaces the previous zero-state BenchmarkRunner with the new "collect-then-run" idiom
in the examples and documentation.

In most of the docs, this is only a syntax change, but some formulations had to be tweaked,
since there is no more single runner resource.
@nicholasjng nicholasjng merged commit c58978c into main Dec 21, 2024
14 checks passed
@nicholasjng nicholasjng deleted the add-concurrency branch December 21, 2024 15:47
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

Successfully merging this pull request may close these issues.

1 participant