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

[Compiler] Integrate with Catalyst Frontend #4692

Merged
merged 34 commits into from Nov 28, 2023
Merged

Conversation

maliasadi
Copy link
Member

@maliasadi maliasadi commented Oct 18, 2023

Context:
We implement a compiler module that provides the PennyLane's native JIT compilation support.

This compiler subpackage exclusively functions as a wrapper for PennyLane's JIT compiler packages, without independently implementing any compiler itself. Currently, it supports the 'pennylane-catalyst' package, with plans to incorporate additional packages in the near future.

For any compiler packages seeking to be registered, it is imperative that they expose the 'entry_points' metadata under the designated group name: pennylane.compilers. (Check PR PennyLaneAI/catalyst#321 for details)

Description of the Change:

  • qml.compiler : The JIT compiler module containing all utilities and wrapper methods for qml.qjit in PennyLane.
  • qml.qjit : A just-in-time decorator for PennyLane and JAX programs backed by Catalyst

Benefits: Compiling hybrid programs without explicitly importing Catalyst.

Possible Drawbacks: To use qml.qjit, users need to manually install pennylane-catalyst.

[sc-46400]
[sc-46401]

@github-actions
Copy link
Contributor

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.

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 @maliasadi, I just left some quick comments.

pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thanks @maliasadi, I like this entry point design! I do have some concerns though:

pennylane/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Nice work @maliasadi! I think I mostly have the same comments as @dime10, so once addressed this should be good to go 👍

pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
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.

Some small comments, great PR 👍

tests/test_compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
…e. Add the AvailableCompilers dataclass and update tests
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0aad62b) 99.65% compared to head (30567ea) 99.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4692      +/-   ##
==========================================
- Coverage   99.65%   99.65%   -0.01%     
==========================================
  Files         385      388       +3     
  Lines       34900    34700     -200     
==========================================
- Hits        34780    34579     -201     
- Misses        120      121       +1     

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

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Really nice work @maliasadi -- the compiler module docstring is really well written and structured!

I managed to get mostly through the PR, but haven't had a chance to review qml.qjit yet, will do so tomorrow

doc/code/qml_compiler.rst Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/qjit_api.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler.py Outdated Show resolved Hide resolved
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Looks great from my end @maliasadi, excited to have this in 😍

Should we add a #do not merge label as a reminder not to merge this yet?

pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Show resolved Hide resolved
@maliasadi maliasadi added the do not merge ⚠️ Do not merge the pull request until this label is removed label Oct 25, 2023
maliasadi and others added 2 commits October 25, 2023 16:52
Co-authored-by: Josh Izaac <josh146@gmail.com>
@maliasadi
Copy link
Member Author

⏰ This PR will be merged after the next release of Catalyst (v0.3.2)

maliasadi and others added 2 commits November 6, 2023 21:16
From the doc page of
[pkg_resources](https://setuptools.pypa.io/en/latest/pkg_resources.html):

> Use of pkg_resources is deprecated in favor of
[importlib.resources](https://docs.python.org/3.11/library/importlib.resources.html#module-importlib.resources),
[importlib.metadata](https://docs.python.org/3.11/library/importlib.metadata.html#module-importlib.metadata)
and their backports
([importlib_resources](https://pypi.org/project/importlib_resources),
[importlib_metadata](https://pypi.org/project/importlib_metadata)). Some
useful APIs are also provided by
[packaging](https://pypi.org/project/packaging) (e.g. requirements and
version parsing). Users should refrain from new usage of pkg_resources
and should work to port to importlib-based solutions.

PR #4783 updates all occurrences of `pkg_resources` with
`importlib.metadata` across the entire PL. This PR, however, addresses
this replacement in the compiler driver, which has not yet been merged
into PL.

[sc-46399]
Context:
This PR adds support for QJIT compatible `qml.jacobian` and `qml.grad`.
Please see ADR 74 for implementation details.

[sc-46406]

---------

Co-authored-by: Josh Izaac <josh146@gmail.com>
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

looks great, just a few things I'm curious about. Also, do we have a way of selecting which compiler to use if a user has more than one installed?

pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/_grad.py Outdated Show resolved Hide resolved
pennylane/_grad.py Show resolved Hide resolved
pennylane/_grad.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
tests/test_compiler.py Outdated Show resolved Hide resolved
tests/test_compiler.py Show resolved Hide resolved
tests/test_compiler.py Outdated Show resolved Hide resolved
tests/test_compiler.py Outdated Show resolved Hide resolved
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

i'd fix the active_compiler "Returns" docstring syntax and add the experimental warning to "Adding a compiler" in this PR, but otherwise lgtm!

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 @maliasadi!

doc/introduction/compiling_circuits.rst Show resolved Hide resolved
pennylane/_grad.py Show resolved Hide resolved
pennylane/_grad.py Outdated Show resolved Hide resolved
pennylane/_grad.py Outdated Show resolved Hide resolved
pennylane/_grad.py Outdated Show resolved Hide resolved
pennylane/compiler/__init__.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/compiler.py Outdated Show resolved Hide resolved
pennylane/compiler/qjit_api.py Outdated Show resolved Hide resolved
pennylane/compiler/qjit_api.py Show resolved Hide resolved
…#4812)

This PR updates the support and addresses code review suggestions in PR
#4692.

- [x] Activate integration tests in CI (using Catalyst v0.3.2)
- [x] Fix docstring issues in the compiler and grad docs
- [x] Use `h` instead of `step_size` in grad ops
- [x] Update code coverage of the compiler module
@maliasadi maliasadi removed the do not merge ⚠️ Do not merge the pull request until this label is removed label Nov 27, 2023
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Show resolved Hide resolved
@maliasadi maliasadi merged commit 34a561a into master Nov 28, 2023
35 checks passed
@maliasadi maliasadi deleted the maa/integrate-catalyst-2 branch November 28, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants