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

Create VJP perf fix #4806

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Create VJP perf fix #4806

merged 6 commits into from
Nov 8, 2023

Conversation

mlxd
Copy link
Member

@mlxd mlxd commented Nov 8, 2023

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    test directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the doc/releases/changelog-dev.md file, summarizing the
    change, and including a link back to the PR.

  • The PennyLane source code conforms to
    PEP8 standards.
    We check all of our code against Pylint.
    To lint modified files, simply pip install pylint, and then
    run pylint pennylane/path/to/file.py.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context: v0.33.1 backport of PR #4792

Description of the Change: Updates VJP pipeline to favour direct matrix-vector products where possible.

Benefits: Improves performance for many parameter/many observable workloads

Possible Drawbacks:

Related GitHub Issues: #4792

Copy link
Contributor

github-actions bot commented Nov 8, 2023

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@mlxd mlxd added gpu ci:run-full-test-suite Run the full test-suite on the pull request labels Nov 8, 2023
Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @mlxd @albi3ro! Nothing blocking from my side but I do have a few questions.

pennylane/gradients/vjp.py Show resolved Hide resolved
pennylane/gradients/vjp.py Show resolved Hide resolved
pennylane/gradients/vjp.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b20871) 99.64% compared to head (4136300) 99.64%.
Report is 2 commits behind head on v0.33.1-rc0.

Additional details and impacted files
@@             Coverage Diff              @@
##           v0.33.1-rc0    #4806   +/-   ##
============================================
  Coverage        99.64%   99.64%           
============================================
  Files              380      380           
  Lines            34266    34285   +19     
============================================
+ Hits             34145    34164   +19     
  Misses             121      121           
Files Coverage Δ
pennylane/_version.py 100.00% <100.00%> (ø)
pennylane/devices/qubit/simulate.py 100.00% <100.00%> (ø)
pennylane/gradients/vjp.py 100.00% <100.00%> (ø)
pennylane/math/multi_dispatch.py 99.59% <100.00%> (+<0.01%) ⬆️
pennylane/ops/op_math/symbolicop.py 100.00% <100.00%> (ø)
pennylane/transforms/defer_measurements.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! It would be nice to have the list of cases where the it becomes slower because of the fallback. What I can identify is the following:

The performance is better:

  1. Single measurement (not tensorflow) with parameters of the same shape.
  2. Multiple measurements with same shapes and with parameters of the same shape.

The performance is worse:

  1. Parameters of the different shapes.
  2. Tensorflow in general takes a hit because it does not support @ for mulitplication
  3. Mixing measurements with different shapes is slower because of the fallback in the compute_vjp_multi.

In the future we should come back to dispatch the cases properly.

pennylane/gradients/vjp.py Show resolved Hide resolved
pennylane/gradients/vjp.py Show resolved Hide resolved
pennylane/gradients/vjp.py Show resolved Hide resolved
@timmysilv
Copy link
Contributor

try-except generally hurts performance. I made a table of when we need which try-excepts so we can improve this in the future:

non-TF TF
non-ragged none both
ragged multi both

@rmoyard rmoyard self-requested a review November 8, 2023 19:41
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks! We made a small table of what cases need what try expect and it also show which one are better and which one become worse. It will be easier to come back to this issue.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Performance improvements for the vast majority of hpc cases we care about, at the cost of a penality for edge cases and tensorflow. I find that to be an acceptable tradeoff.

While this code could probably always use more fine-tuning, this makes accomplishes exactly what it needs to accomplish,

@mlxd mlxd merged commit f3b41b6 into v0.33.1-rc0 Nov 8, 2023
33 of 37 checks passed
@mlxd mlxd deleted the fix/vjp_perf_ref_v0.33.1 branch November 8, 2023 20:10
timmysilv added a commit that referenced this pull request Nov 15, 2023
Note that these changes have been reviewed and released already, they
just aren't on master.

**Context:**
Bugfix release v0.33.1 had a change that isn't on master, so this gets
them onto master

**Description of the Change:**
All I did was run `git merge v0.33.1`, then resolve conflicts.

**Benefits:**
Docs are in order, and the performance patch is on master.

**Possible Drawbacks:**
N/A

For reference, these are the changes that were added in v0.33.1:
v0.33.0...v0.33.1 -
this PR has less files because all of the little fixes were already on
master. It was only #4806 that never made it to master.

---------

Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Romain Moyard <rmoyard@gmail.com>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Isaac De Vlugt <isaacdevlugt@gmail.com>
Co-authored-by: Isaac De Vlugt <34751083+isaacdevlugt@users.noreply.github.com>
Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
Co-authored-by: Jay Soni <jbsoni@uwaterloo.ca>
Co-authored-by: Lee James O'Riordan <mlxd@users.noreply.github.com>
Co-authored-by: BM7878 <117289949+BM7878@users.noreply.github.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:run-full-test-suite Run the full test-suite on the pull request gpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants