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

[MLIR + Frontend] Quantum Module Abstraction #1144

Merged
merged 53 commits into from
Oct 7, 2024

Conversation

erick-xanadu
Copy link
Contributor

@erick-xanadu erick-xanadu commented Sep 19, 2024

Context: We create nested modules for qnodes that contain all the program for a specific qnode. This will allow us to perform transformations targetting specific qnodes.

Description of the Change:

  • Adds catalyst.launch_kernel that can call functions in nested modules.
  • Adds pass to keep previous behaviour by:
    • determining the name of a function given a scope
    • renaming all functions in nested modules to be unique
    • creating a map from flat symbol names to symbol names with path
    • inlining nested modules
    • transforming catalyst.launch_kernel to func.call

Benefits:

Nothing is currently using this pass / abstraction. But now we have the scaffolding for qnode specific transformations and lowerings.

Possible Drawbacks: More compilation time. Nested nested qnodes semantics are still needed. It was opted to disable tests with nested qnodes as they are not fully supported. Only nested qnodes where no qnode execution overlap was supported previously, but this is not the general case and could easily lead to runtime errors.

Related GitHub Issues:

TODO (minor changes):

  • cleaning up the frontend
  • (optional) upstream changes to symbol table, to avoid patterns on gradient interface and zne.
  • use values from ctx.module_context instead of hard coded ones
  • can we avoid changing the order of functions as they appear? (This would help lit testing)
  • remove func_p?

[sc-67095]

@erick-xanadu erick-xanadu marked this pull request as draft September 19, 2024 15:44
@erick-xanadu erick-xanadu force-pushed the eochoa/2024-09-16/nested-module branch 4 times, most recently from 3bc3bd4 to 6acdf4d Compare September 19, 2024 19:42
@erick-xanadu erick-xanadu marked this pull request as ready for review September 19, 2024 19:43
@erick-xanadu erick-xanadu force-pushed the eochoa/2024-09-16/nested-module branch 2 times, most recently from 009342d to 845e927 Compare September 19, 2024 19:49
@erick-xanadu erick-xanadu force-pushed the eochoa/2024-09-16/nested-module branch from 845e927 to 1f41812 Compare September 19, 2024 20:09
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 99.16667% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.89%. Comparing base (3bea1b6) to head (8e0890e).
Report is 199 commits behind head on main.

Files with missing lines Patch % Lines
frontend/catalyst/jax_primitives.py 98.98% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
+ Coverage   97.86%   97.89%   +0.03%     
==========================================
  Files          76       76              
  Lines       10819    10888      +69     
  Branches     1283     1290       +7     
==========================================
+ Hits        10588    10659      +71     
+ Misses        179      178       -1     
+ Partials       52       51       -1     

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

@erick-xanadu erick-xanadu marked this pull request as draft September 19, 2024 21:26
@erick-xanadu erick-xanadu marked this pull request as draft September 19, 2024 21:26
@erick-xanadu erick-xanadu force-pushed the eochoa/2024-09-16/nested-module branch from 2979c40 to a9e0c6b Compare September 20, 2024 19:07
@erick-xanadu erick-xanadu force-pushed the eochoa/2024-09-16/nested-module branch from a9e0c6b to 97f4771 Compare September 20, 2024 19:09
@erick-xanadu erick-xanadu marked this pull request as ready for review September 20, 2024 19:56
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.

Nice work 💯 I left some suggestions regarding the frontend which relate to a lot of your Todos as well I think :)

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
frontend/catalyst/compiler.py Show resolved Hide resolved
frontend/catalyst/compiler.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
mlir/lib/Catalyst/Transforms/InlineNestedModules.cpp Outdated Show resolved Hide resolved
mlir/lib/Catalyst/Transforms/InlineNestedModules.cpp Outdated Show resolved Hide resolved
mlir/lib/Catalyst/Transforms/InlineNestedModules.cpp Outdated Show resolved Hide resolved
mlir/lib/Catalyst/Transforms/InlineNestedModules.cpp Outdated Show resolved Hide resolved
mlir/test/Catalyst/NestedModule5.mlir Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
@paul0403
Copy link
Contributor

paul0403 commented Sep 26, 2024

Thanks! 🥇

An unrelated comment: once this qnode module scope is in place I'd like to utilize it and convert how we schedule passes in transform_named_sequence. No need to do that in this PR.

@erick-xanadu erick-xanadu requested a review from dime10 October 1, 2024 20:47
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.

Nice work 💯 I'm happy to approve pending resolution of the open comments :)

frontend/catalyst/from_plxpr.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py Outdated Show resolved Hide resolved
frontend/catalyst/jax_primitives.py 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.

🦝 🚀 🌔

@erick-xanadu erick-xanadu merged commit aeb1e43 into main Oct 7, 2024
43 checks passed
@erick-xanadu erick-xanadu deleted the eochoa/2024-09-16/nested-module branch October 7, 2024 13:19
rauletorresc pushed a commit that referenced this pull request Oct 9, 2024
**Context:** We create nested modules for qnodes that contain all the
program for a specific qnode. This will allow us to perform
transformations targetting specific qnodes.

**Description of the Change:**

* Adds `catalyst.launch_kernel` that can call functions in nested
modules.
* Adds pass to keep previous behaviour by:
  * determining the name of a function given a scope
  * renaming all functions in nested modules to be unique
  * creating a map from flat symbol names to symbol names with path
  * inlining nested modules
  * transforming `catalyst.launch_kernel` to `func.call`

**Benefits:**

Nothing is currently using this pass / abstraction. But now we have the
scaffolding for qnode specific transformations and lowerings.

**Possible Drawbacks:** More compilation time. Nested nested qnodes
semantics are still needed. It was opted to disable tests with nested
qnodes as they are not fully supported. Only nested qnodes where no
qnode execution overlap was supported previously, but this is not the
general case and could easily lead to runtime errors.

**Related GitHub Issues:**

[sc-67095]

---------

Co-authored-by: David Ittah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants