Skip to content

Python: add bindings for AddCustomContext #1988

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Python: add bindings for AddCustomContext #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bindings/python/google_benchmark/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,5 @@ def main(argv=None):
# Methods for use with custom main function.
initialize = _benchmark.Initialize
run_benchmarks = _benchmark.RunSpecifiedBenchmarks
add_custom_context = _benchmark.AddCustomContext
atexit.register(_benchmark.ClearRegisteredBenchmarks)
4 changes: 4 additions & 0 deletions bindings/python/google_benchmark/benchmark.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,5 +180,9 @@ NB_MODULE(_benchmark, m) {
m.def("RunSpecifiedBenchmarks",
[]() { benchmark::RunSpecifiedBenchmarks(); });
m.def("ClearRegisteredBenchmarks", benchmark::ClearRegisteredBenchmarks);
m.def("AddCustomContext", benchmark::AddCustomContext, nb::arg("key"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not bind this directly as add_custom_context? That might also be IDE/completions-friendlier than the indirection by the above reassignment.

(I know you did it to match the rest of the examples, but in my opinion it's worth asking whether that convention should be kept.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know of one subtle reason behind this. There's documentation for "AddCustomContext" and one can find it in __init__.py, which is easily accessible from IDE. Is it good enough reason? Maybe not. Maybe there should be good docstrings for every method. Currently, I'd give this issue low priority. For me, the lack of type annotations for most methods is much more important. And maybe the lack of format checking for (google_benchmark/benchmark.cc) in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my answer is: convention should NOT be kept, and change of convention is not part of this issue.

nb::arg("value"),
"Add a key-value pair to output as part of the context stanza in the "
"report.");
};
} // namespace
2 changes: 2 additions & 0 deletions bindings/python/google_benchmark/example.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import random
import time
import sys

import google_benchmark as benchmark
from google_benchmark import Counter
Expand Down Expand Up @@ -137,4 +138,5 @@ def computing_complexity(state):


if __name__ == "__main__":
benchmark.add_custom_context("python", sys.version)
benchmark.main()
Loading