Add script to compare commits and additional benchmarks #568

johannesjmeyer · 2020-04-01T14:16:04Z

Context:
The PR #567 needs some tests to see if it works.

Description of the Change:
This PR introduces a script to benchmark across commits.
Furthermore it adds new benchmarks.

There now exists a script benchmark_revisions.py that accepts a list of git revisions (i.e. branches, tags or commits). It downloads those commits and then runs benchmark.py with all the other parameters it got using the downloaded revision of pennylane.

Revisions are stored to decrease the number of necessary downloads.

Furthermore, a new benchmark bm_iqp_circuit, benchmarking mostly diagonal gates, was added and other benchmarks fixed.

codecov · 2020-04-01T14:18:41Z

Codecov Report

Merging #568 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #568   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files          82       82           
  Lines        5124     5124           
=======================================
  Hits         5069     5069           
  Misses         55       55

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7b3b37...a7b3b37. Read the comment docs.

co9olguy · 2020-04-01T17:31:29Z

This will be a great tool @johannesjmeyer!

co9olguy · 2020-04-24T12:50:14Z

I'm curious about some of the underlying logic. This script only works for revisions which are on the remote server, correct? It doesn't seem to recognize any commits/branches I have locally

johannesjmeyer · 2020-04-24T13:12:49Z

I now also fixed some other git stuff that caused the wrong revision to be downloaded and that fixed the download of revisions that are "too new".

@co9olguy Yes, the script only runs for revisions on the remote server. I wouldn't necessarily know where to look on your local machine. One could of course add this possibility and give a directory, but I don't think that this merits the work

@josh146 I didn't put it into tmp (which is possible platform-independently) because I want the files to rest there (otherwise I have to clone them anytime I benchmark something which might take long). Do you think I should include a --clean switch that removes the folder?

co9olguy · 2020-04-24T13:30:06Z

@co9olguy Yes, the script only runs for revisions on the remote server. I wouldn't necessarily know where to look on your local machine. One could of course add this possibility and give a directory, but I don't think that this merits the work

Since you mentioned above that the benchmark should be run out of the pennylane "mother" directory, and that will always be itself a git repository, then it should be enough to just query from the benchmark folder itself, no?

johannesjmeyer · 2020-04-24T13:45:38Z

Since you mentioned above that the benchmark should be run out of the pennylane "mother" directory, and that will always be itself a git repository, then it should be enough to just query from the benchmark folder itself, no?

Yes, I will add "here" as a special command that uses git to determine the root of your current git folder.

co9olguy

Nice, thanks @johannesjmeyer, looking forward to giving this tool some use :)

Everything is now working as advertised. The only thing I wonder is if it is necessary to create separate folders for every revision during testing. Couldn't we make one folder (or perhaps just use the "mother" folder!), then just use git to switch between the different commits?

.github/CHANGELOG.md

benchmark/README.rst

co9olguy · 2020-04-24T13:34:23Z

benchmark/README.rst

+
+  python3 benchmark_revisions.py -r master,0c8e90a -d default.qubit,default.tensor time bm_mutable_rotations
+
+The chosen revisions will be downloaded and cached into the ``revisions`` subdirectory of the benchmarking folder.


Now goes to home directory, correct?

benchmark/bm_iqp_circuit.py

co9olguy · 2020-04-24T13:52:01Z

benchmark/benchmark_revisions.py

+                    # PL repository. Instead we just copy the first revision in the directory
+                    # checkout will then get the correct revision
+                    first_revision = next((x for x in revisions_directory.iterdir() if x.is_dir()))
+                    print(">>> Revision not found locally, copying...")


Suggested change

print(">>> Revision not found locally, copying...")

print(">>> Revision found locally, copying...")

co9olguy · 2020-04-24T13:57:04Z

benchmark/benchmark_revisions.py

+                                revision,
+                                "-q",
+                            ],
+                            check=True,


This is what allowed me to figure out the --noinfo error I was seeing. Otherwise I think it would have silently failed

co9olguy · 2020-04-24T13:58:41Z

benchmark/benchmark_revisions.py

+                with cd(pl_directory):
+                    subprocess.run(["git", "checkout", "master", "-q"], check=True)
+                    subprocess.run(["git", "fetch", "-q"], check=True)
+                    res = subprocess.run(["git", "checkout", revision, "-q"])


no check=True here? Is that because you want to inspect res.returncode below?

co9olguy · 2020-04-24T14:00:58Z

benchmark/benchmark_revisions.py

+            benchmark_env = os.environ.copy()
+            benchmark_env["PYTHONPATH"] = str(pl_directory) + ";" + benchmark_env["PATH"]
+            subprocess.run(
+                ["python3", str(benchmark_file_path)] + unknown_args + ["--noinfo"],


so "--noinfo" is always turned on for benchmarking revisions?

co9olguy · 2020-04-24T14:01:23Z

benchmark/benchmark_revisions.py

+            print(
+                col(
+                    ">>> An error occured during execution of the script. "
+                    + "Deleting the current revision folder to not leave junk.",


Suggested change

+ "Deleting the current revision folder to not leave junk.",

+ "Deleting the current revision folder.",

co9olguy · 2020-04-24T14:02:12Z

benchmark/benchmark_revisions.py

+                env=benchmark_env,
+                check=True,
+            )
+        except:


Will section ever get with check=True? (I'm not too experienced with using subprocess.run, just checking

antalszava

Looks good to me! 🚀 Thanks @johannesjmeyer

Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>

josh146

Looks good to me, and tested locally and still works well :)

josh146 · 2020-04-30T12:33:59Z

benchmark/README.rst

 The revisions -- including branches, tags and commits -- are specified via ``-r revision1[,revision2[,revision3...]]``,
 as in

 .. code-block:: bash

  python3 benchmark_revisions.py -r master,0c8e90a -d default.qubit,default.tensor time bm_mutable_rotations

-The chosen revisions will be downloaded and cached into the ``revisions`` subdirectory of the benchmarking folder.
+The chosen revisions will be downloaded and cached into the ``pennylane.benchmark/revisions`` subdirectory of your user home directory.


looks like it's now .pennylane/benchmarks/revisions?

co9olguy · 2020-04-30T16:26:17Z

Thanks @johannesjmeyer, i am happy to merge this in (I had already approved).

Not something that's important for this PR, but I'm still a bit puzzled why we can't just perform the benchmarking in-place. What is the requirement that forces us to create the new folder $HOME/.pennylane/benchmarks/revisions, which contains an entire copy of the pennylane codebase, rather than just checking out the commits from within the user's active pennylane folder (which we can assume is already version-controlled)?

co9olguy · 2020-04-30T18:47:49Z

I guess the primary reason is that we wouldn't be able to test any commits that didn't yet have the benchmark subfolder? Is that correct?

antalszava · 2020-05-01T19:47:37Z

benchmark/benchmark_revisions.py

+            # Make really sure we don't reset the current git
+            if toplevel == Path.cwd():
+                raise Exception(
+                    "Git accidently ended up in the current directory. Stopping to not cause any harm."


antalszava · 2020-05-01T19:48:32Z

Nice, working as before! 😉 👍

johannesjmeyer · 2020-05-02T15:46:28Z

I guess the primary reason is that we wouldn't be able to test any commits that didn't yet have the benchmark subfolder? Is that correct?

No, actually my principal reason is that I really don't want to mess with the current git. I mean, we could surely implement some logic that stashes pending changes if there are some and then recovers them. But imagine there is some error during the script execution and your changes are gone. So I figured it is better to do it in a sandboxed environment. Also PennyLane source is 43MB, this is not hurting anyone.

johannesjmeyer added 3 commits April 1, 2020 16:13

Remove benchmark folder from gitignore

06c6942

Add IQP circuit benchmark

c13e73a

Black

bdbcd54

johannesjmeyer added WIP 🚧 Work-in-progress performance ⏲️ Benchmarking and performance improvements labels Apr 1, 2020

Add docstrings

59af7ae

johannesjmeyer changed the title ~~[WIP] Add additional benchmarks, script to compare commits~~ [WIP] Add script to compare commits and additional benchmarks Apr 1, 2020

johannesjmeyer added 3 commits April 1, 2020 18:31

Add first script draft

1dfbd6c

Fix most of the script

467674e

Add todo comment

10c00ea

johannesjmeyer added 18 commits April 2, 2020 10:57

Use subprocess to run benchmark

9b02c41

Add todo

5029b5c

Add note

7325db5

Rename commit to revision

0d56e9c

Also rename file to reflect namechange

de65673

Use git to download PL

e7d1c13

Fix rest of code

22f7af9

Update gitignore

998c3e2

Update todos

7b41587

Rename directory to pl_directory

8ceedcf

Fix benchmark script call

cbbb069

Remove master directory

d8311ca

Add --noinfo for benchmark.py

cfd2af1

Add color and remove superfluous context

edc5873

Update gitignore

184a959

Update printing

a0e801c

Black

6ed20e9

I dont know why these keep appearing

54bd4be

Fix git

2b47d39

johannesjmeyer added 2 commits April 24, 2020 16:01

Add here option

f4782a2

Mention here in docs

23413f8

co9olguy approved these changes Apr 24, 2020

View reviewed changes

johannesjmeyer added 2 commits April 24, 2020 16:17

Refactor

b27b7fc

Codefactor

9cb8c69

antalszava approved these changes Apr 24, 2020

View reviewed changes

co9olguy and others added 8 commits April 24, 2020 12:27

Merge branch 'master' into benchmark_commits

b541192

Merge branch 'master' into benchmark_commits

5ef6562

Simplify script

8af8054

Remove unused imports

9cd6228

Add failsafes to not reset current branch

c546f20

Black

77e05ad

Apply suggestions from code review

834758d

Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>

Remove duplicate code

4d90b08

josh146 approved these changes Apr 30, 2020

View reviewed changes

Merge branch 'master' into benchmark_commits

2512129

Merge branch 'master' into benchmark_commits

a7b3b37

antalszava reviewed May 1, 2020

View reviewed changes

johannesjmeyer merged commit 3d52b4b into master May 2, 2020

johannesjmeyer deleted the benchmark_commits branch May 2, 2020 15:46

co9olguy mentioned this pull request May 8, 2020

Update benchmarking suite to support running on older commits #628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to compare commits and additional benchmarks #568

Add script to compare commits and additional benchmarks #568

johannesjmeyer commented Apr 1, 2020 •

edited

codecov bot commented Apr 1, 2020 •

edited

co9olguy commented Apr 1, 2020

co9olguy commented Apr 24, 2020

johannesjmeyer commented Apr 24, 2020

co9olguy commented Apr 24, 2020

johannesjmeyer commented Apr 24, 2020

co9olguy left a comment

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

co9olguy Apr 24, 2020

antalszava left a comment

josh146 left a comment

josh146 Apr 30, 2020

co9olguy Apr 30, 2020 •

edited

co9olguy commented Apr 30, 2020

co9olguy commented Apr 30, 2020

antalszava May 1, 2020

antalszava commented May 1, 2020

johannesjmeyer commented May 2, 2020


		python3 benchmark_revisions.py -r master,0c8e90a -d default.qubit,default.tensor time bm_mutable_rotations

		The chosen revisions will be downloaded and cached into the ``revisions`` subdirectory of the benchmarking folder.

	print(">>> Revision not found locally, copying...")
	print(">>> Revision found locally, copying...")

	+ "Deleting the current revision folder to not leave junk.",
	+ "Deleting the current revision folder.",

Add script to compare commits and additional benchmarks #568

Add script to compare commits and additional benchmarks #568

Conversation

johannesjmeyer commented Apr 1, 2020 • edited

codecov bot commented Apr 1, 2020 • edited

Codecov Report

co9olguy commented Apr 1, 2020

co9olguy commented Apr 24, 2020

johannesjmeyer commented Apr 24, 2020

co9olguy commented Apr 24, 2020

johannesjmeyer commented Apr 24, 2020

co9olguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

co9olguy Apr 30, 2020 • edited

Choose a reason for hiding this comment

co9olguy commented Apr 30, 2020

co9olguy commented Apr 30, 2020

Choose a reason for hiding this comment

antalszava commented May 1, 2020

johannesjmeyer commented May 2, 2020

johannesjmeyer commented Apr 1, 2020 •

edited

codecov bot commented Apr 1, 2020 •

edited

co9olguy Apr 30, 2020 •

edited