Pytest benchmark override with cpu and gpu times #21

MrRobot2211 · 2021-07-07T15:43:59Z

The idea here is to override pytest_benchmark fixture calls to retain most of the funtionality but use cupy.benchmark.repeat for timings instead of pytest_benchmark's internal runner object. This allows us to benchmark total, cpu and gpu times. I am striving for a fast solution, the sooner we can start benchmarking the better.

Maybe, this could done more elegantly by using a cutom timer probably
, but then one would run into trouble elsewhere in the code, and may de forced to create a wrapper calls for the oputput of the timer with an specialized __add__ method and to change the library used for statistics. At that point registerin our own pytest plugin that links to pytest_benchmark maybe more suitable. That looks more like a long-term implementation for later on.

To quick test that the functionality works you should install pytest_benchmark and do:

python src/benchmark_tools/benchmark.py

and you will be shown three different statistics: full time, cpu time and gpu time.

you can also build a json and see the separate statistics, this will allow it to play well with Asier's benchmarking code.

Why it works

In the Fixture class the method _add_stats appends a new benchmark (bench_stats of class Metadata) into a BenchmarkSession list.
Each element of this list will then be taken one by one the as_dict method and dumped into a json file.
No runtime checks are performed in pytest_benchmark on the size of this list.
Note that each bench_stats keepss the group and extra information from the Fixture wherein it was created.

By copying the structur in __call__/ pedantic for our new funtion, iinstead of acreating a bench_stats with each call we will be creating three of them with different group attributes. Whose statistics will be calculated separately.

Pending tasks

Properly document the relevant behaviour changes from the original pytest_benchmark functionality.
Cleaning up and formatting the code
Add functionalitry to register the GPU name and features being used for the test.

I am open to add more things to the list as the review progresses

Other similar packages

rapids-pytest-benchmark implements a similar solution to ours for wrapping the benchmark module in pytest-benchmark it adds functionality for benchmarking memory use but it lacks host and device separate stats.
cupy-performance does something very similar to what we want but in a unittest style.

coveralls · 2021-07-07T15:53:23Z

Pull Request Test Coverage Report for Build 1070994715

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 82.667%

Totals
Change from base Build 1025096836:	0.0%
Covered Lines:	62
Relevant Lines:	75

💛 - Coveralls

…t_benchmark_override_with_cpu_and_gpu_times

hodgestar · 2021-07-13T12:25:51Z

benchmarks/cpu_gpu_times_wrapper.py

+    def __setattr__(self, attr, value):
+        setattr(self.wrapped_class, attr, value)


I think we can safely remove __setattr__.

Setting __setattr__ causes self.x = y to always call __setattr__ and setting an attribute on self has to be done via self.__dict__["x"] = y (which is painful and easy to forget to do). On top of this, its hard to imagine why someone doing self.x = y on the wrapper object would want the attribute to be set on the wrapped class.

I need things to be set on the wrapped class because it is the one that does all the communications with the pytest benchmark modules. The wrapper class should not have any attributes besides the class being wrapped. Maybe there is an easier way to dinamically add this functionality on the classes ?

I think that the rapids version, which does not have the __setattr__ may have issues ( specially if we register it like a pytest plugin) when manually setting attributes , like in Asier's code, which I would like to reuse in part.

Notice that if we were to register our wrapper to pytest , when doing benchmark.groups = "x" we wouldn't be setting the wrapped groups attribute, which then gets pushed to the BenchmarkSession.

hodgestar · 2021-07-13T12:55:30Z

Just linking to the source code of the pytest-benchmark class we're wrapping for those who are reading along and reviewing: https://github.com/ionelmc/pytest-benchmark/blob/master/src/pytest_benchmark/fixture.py

compatible with pedantic

hodgestar · 2021-08-03T11:19:18Z

@MrRobot2211 What is the status of this PR? Should I review again? Or would you like to make more changes first?

global variables. The structure had to be changed for this to work alternatively the structure could be kept if setup.cfg is changed to recognize the extra-module

…t_benchmark_override_with_cpu_and_gpu_times

MrRobot2211 · 2021-08-17T06:54:21Z

@hodgestar I am missing here a tiny little code to generalize benchmarks to all operations, but I think it is a good idea that you take a look. I am not very comfortable about having promoted benchmark-tools to a module but did not see any better option.

MrRobot2211 · 2021-10-26T16:06:53Z

@hodgestar I am missing here a tiny little code to generalize benchmarks to all operations, but I think it is a good idea that you take a look. I am not very comfortable about having promoted benchmark-tools to a module but did not see any better option.

@hodgestar just a remainder.

hodgestar · 2021-10-26T16:32:45Z

src/benchmark_tools/cpu_gpu_times_wrapper.py

@@ -0,0 +1,158 @@
+import warnings
+
+# from pytest_benchmark.fixture import BenchmarkFixture


Delete this commented out line.

hodgestar

I left a couple of comments. Do you have example output from the benchmark runs? Perhaps we can add that as part of this PR somehow.

hodgestar · 2021-10-26T16:33:27Z

src/benchmark_tools/cpu_gpu_times_wrapper.py

+    utility ``cupyx.time.repeat``
+    """
+
+    def __init__(self, wrapped_class, iterations=30, rounds=5, warmup_rounds=10):


I think "wrapped_class" is really just a "wrapped_obj" here, so we should name it appropriately.

hodgestar · 2021-10-26T16:33:38Z

src/benchmark_tools/cpu_gpu_times_wrapper.py

+        self.warmup_rounds = warmup_rounds
+
+    def __getattr__(self, attr):
+        # orig_attr = self.wrapped_class.__getattribute__(attr)


Also delete this commented out code.

MrRobot2211 added 5 commits July 7, 2021 10:52

basic directory structure

7d601f0

adds basic implementation for wrapper

6d27ed4

adds basic test functionality

5eb0159

corrects pedantioc cupy arguments

8262871

working first example

0986f40

hodgestar mentioned this pull request Jul 12, 2021

Benchmarks #11

Open

Merge branch 'main' of https://github.com/qutip/qutip-cupy into pytes…

5c913a6

…t_benchmark_override_with_cpu_and_gpu_times

hodgestar reviewed Jul 13, 2021

View reviewed changes

MrRobot2211 added 3 commits July 27, 2021 03:17

make pedanticupy arguments

c6352f8

compatible with pedantic

add benchmark plot generation

fef8bec

add groups dynamically in benchmark tests

a6217ca

MrRobot2211 added 11 commits August 16, 2021 04:52

working with one benchmark

7adf443

add device_id settings as settable

357db49

global variables. The structure had to be changed for this to work alternatively the structure could be kept if setup.cfg is changed to recognize the extra-module

fix benchmark device setup

48dcb16

add array creation by dtype

9da6dad

Merge branch 'main' of https://github.com/qutip/qutip-cupy into pytes…

9d541b5

…t_benchmark_override_with_cpu_and_gpu_times

aadd device half precision option

8f0d041

format

f950d20

format

1d647ba

add benchmarks to setup.cfg

bda4d2b

correct plot labeling

f4006ef

add extra-dependencies for benchmark

7eac576

hodgestar reviewed Oct 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pytest benchmark override with cpu and gpu times #21

Pytest benchmark override with cpu and gpu times #21

MrRobot2211 commented Jul 7, 2021 •

edited

Loading

coveralls commented Jul 7, 2021 •

edited

Loading

hodgestar Jul 13, 2021

MrRobot2211 Jul 13, 2021 •

edited

Loading

hodgestar commented Jul 13, 2021

hodgestar commented Aug 3, 2021

MrRobot2211 commented Aug 17, 2021

MrRobot2211 commented Oct 26, 2021

hodgestar Oct 26, 2021

hodgestar left a comment

hodgestar Oct 26, 2021

hodgestar Oct 26, 2021

		def __setattr__(self, attr, value):
		setattr(self.wrapped_class, attr, value)

		@@ -0,0 +1,158 @@
		import warnings

		# from pytest_benchmark.fixture import BenchmarkFixture

Pytest benchmark override with cpu and gpu times #21

Are you sure you want to change the base?

Pytest benchmark override with cpu and gpu times #21

Conversation

MrRobot2211 commented Jul 7, 2021 • edited Loading

Why it works

Pending tasks

Other similar packages

coveralls commented Jul 7, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1070994715

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

hodgestar Jul 13, 2021

Choose a reason for hiding this comment

MrRobot2211 Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

hodgestar commented Jul 13, 2021

hodgestar commented Aug 3, 2021

MrRobot2211 commented Aug 17, 2021

MrRobot2211 commented Oct 26, 2021

hodgestar Oct 26, 2021

Choose a reason for hiding this comment

hodgestar left a comment

Choose a reason for hiding this comment

hodgestar Oct 26, 2021

Choose a reason for hiding this comment

hodgestar Oct 26, 2021

Choose a reason for hiding this comment

MrRobot2211 commented Jul 7, 2021 •

edited

Loading

coveralls commented Jul 7, 2021 •

edited

Loading

MrRobot2211 Jul 13, 2021 •

edited

Loading