feat: build autoqasm program directly from main decorator #804

ajberdy · 2023-11-17T01:26:45Z

Issue #, if available:

Fixes #805, fixes #806, fixes #813

Description of changes:

Update aq.main to build AutoQASM program directly.
Add support for typed input parameters and auto-capture of inputs.
- usable in qubit indices
Various typing simplifications

Wrapped testing programs in fixtures so they don't get built during other tests.

Testing done:

Unit tests updated and passing, notebook tests passing as well.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ajberdy · 2023-11-17T22:30:50Z

Two tests failing in test_api: test_nested_function and test_main_from_main

laurencap

Reviewed test_api.py

test/unit_tests/braket/experimental/autoqasm/test_api.py

laurencap · 2023-11-18T02:46:06Z

test/unit_tests/braket/experimental/autoqasm/test_api.py


-    assert main(4).to_ir() == expected
+    assert main.make_bound_program(param_values={"n": 4}).to_ir() == expected


Since these weren't free parameters before, would you mind updating most of the tests to have the meta-programming style like you showed in your doc, with the outer builder function?

That way we can see what the new syntax looks like. I don't think make_bound_program is the recommended way for customers to pass these meta programming inputs

Ah I see. In my head, I interpreted these tests as testing the user story "customer wants to define this program and then build the program with a specific value for its input". I agree make_bound_program is a bit clunky today for this, but we can add easier methods for Program -> Program maps that bind variables, either by simply replacing the input declaration with an assignment, or fully inlining as an optimization pass.

If instead these tests were meant to test the codepath "non-input python variables are used during the transpilation step", then I can see how your suggestion is more relevant. It's not obvious to me which is the case, maybe we can chat so I can better understand here

From sync review: We will update with the second interpretation

test/unit_tests/braket/experimental/autoqasm/test_api.py

rmshaffer · 2023-11-28T20:56:17Z

examples/autoqasm/1_Getting_started_with_AutoQASM.ipynb

Just a note, please also run tox -e notebooks to test the service-dependent example notebooks that currently don't run in CI.

Ah, thanks for pointing this out! Just confirmed that it passes with the latest updates; I'll note this in the PR description

codecov · 2023-11-28T22:14:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8cdf8a4) 100.00% compared to head (4406d4c) 100.00%.
Report is 7 commits behind head on feature/autoqasm.

Additional details and impacted files

@@                Coverage Diff                 @@
##           feature/autoqasm      amazon-braket/amazon-braket-sdk-python#804   +/-   ##
==================================================
  Coverage            100.00%   100.00%           
==================================================
  Files                   166       166           
  Lines                  9895      9921   +26     
  Branches               2102      2107    +5     
==================================================
+ Hits                   9895      9921   +26

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

laurencap · 2023-11-29T18:08:13Z

test/unit_tests/braket/experimental/autoqasm/test_transpiler.py

-    expected = """OPENQASM 3.0;
-qubit[4] __qubits__;
+    expected_no_arg_partial = """OPENQASM 3.0;
+qubit[2] __qubits__;


The qubit length needs to be max qubit index + 1

Suggested change

qubit[2] __qubits__;

qubit[4] __qubits__;

Do we have any plans to validate this? This scenario seems like an example of when we have sufficient information to assert this

laurencap · 2023-11-29T18:10:54Z

test/unit_tests/braket/experimental/autoqasm/test_transpiler.py

 h __qubits__[1];
 cnot __qubits__[1], __qubits__[3];"""

-    bell_partial = aq.main(functools.partial(bell, 1))
-    assert bell_partial(3).to_ir() == expected
+    bell_partial = aq.main(num_qubits=2)(functools.partial(bell, 1))


Why do we have to supply num_qubits on this input now? Maybe it's a rare enough use case that we don't have to worry about it. @rmshaffer, thoughts?

Here we need num_qubits because the program is built with variable qubit indices in use. For the case below, where we pass in a partial fully binding the qubit args, num_qubits can be omitted. This seems correct to me.

It's worth noting that this function previously always built the Program with all variables bound, even though the aq.main call was on varying partials. Now that building happens during the decorator call, incomplete partials will have free parameters.

We could test partials on a meta-programming approach that would more closely match the end result of the previous test, but I think this approach is more fitting, as the essence of this test was asserting the behavior of decorating partial functions.

Agreed, this new behavior seems reasonable and the test makes sense to me.

laurencap · 2023-11-29T18:14:08Z

test/unit_tests/braket/experimental/autoqasm/test_transpiler.py


    a = MyClass()
-    assert aq.main(a.bell)(1, 3).to_ir() == expected
-    assert a.bell_decorated(1, 3).to_ir() == expected
+    assert aq.main(num_qubits=2)(a.rx).to_ir() == expected


Shouldn't rx be called with q0?

This line passes the function a.rx to the decorator; the function isn't actually called at all. You can see from the expected ir that q0 is captured from the signature as an input. Regardless, I will delete this test as mentioned above

laurencap · 2023-11-29T18:14:15Z

test/unit_tests/braket/experimental/autoqasm/test_transpiler.py

-    bell_noarg_partial = functools.partial(bell_decorated, 1, 3)
-    assert bell_noarg_partial().to_ir() == expected
+    bell_noarg_partial = aq.main(num_qubits=2)(functools.partial(bell, 1, 3))
+    assert bell_noarg_partial.to_ir() == expected_no_arg_partial


 def test_classmethod() -> None:


@rmshaffer @yitchen-tim is this important functionality? It seems like we lose this feature, of AutoQASM instance and class methods, if we go this route

Here is some context from Ryan's comment on this test:

I believe I added these class method tests primarily for getting to 100% code coverage in transpiler, and since it was pretty easy to make it work in the old model.

I think it's legitimate to say that this no longer makes sense in the world where the program conversion happens at function declaration time, and we don't need to support this.

I don't think it's important. A user can still easily make a class method which wraps an @aq.main function and returns the Program (which is essentially what it previously meant to mark an instance/class method as @aq.main).

test/unit_tests/braket/experimental/autoqasm/test_transpiler.py

laurencap

Reviewed through notebook 4

examples/autoqasm/3_Iterative_phase_estimation.ipynb

examples/autoqasm/4_Native_programming.ipynb

rmshaffer · 2023-11-30T16:10:51Z

@ajberdy what's the current status of this PR... seems like it's getting close? If you could please just ping @laurencap and me when all of the TODOs/comments have been addressed (or have issues opened) and you are ready for a final PR review pass.

ajberdy · 2023-11-30T18:13:02Z

@ajberdy what's the current status of this PR... seems like it's getting close? If you could please just ping @laurencap and me when all of the TODOs/comments have been addressed (or have issues opened) and you are ready for a final PR review pass.

@rmshaffer Yes, looks like it's getting close! There are 3 TODOs on this PR, all tagged with issues and to be fixed in later PRs (#809, amazon-braket/autoqasm#14, amazon-braket/autoqasm#7). I believe @laurencap has a couple example notebooks left to review and possibly some remaining test files.

I've responded and acted on all the comments, so there aren't currently any action items for the PR on my end. Once all the files are reviewed, I'd say we're good for a final pass.

rmshaffer

A few very minor comments. Overall looks good to me, approving from my end but also deferring to @laurencap after she has a chance to re-review.

rmshaffer · 2023-11-30T19:40:29Z

examples/autoqasm/5_Pulse_programming_and_dynamical_decoupling.ipynb

@@ -69,8 +69,12 @@
   "metadata": {},
   "outputs": [],
   "source": [
+    "# TODO: determine if this example should make these variables inputs to the program\n",


Reminder to remove this TODO before merging. IMO, keeping these as Python variables is fine. (Actually I'm not sure how you'd make qubit an input given how it's used in a format string.)

Technically, should work with qubit as an int. But agree that's not as straightforward as using it directly

rmshaffer · 2023-11-30T19:44:16Z

src/braket/experimental/autoqasm/api.py

+        Program | partial: A callable
+        which returns the converted quantum program when called.


Suggested change

Program | partial: A callable

which returns the converted quantum program when called.

Program | partial: The Program object containing the converted quantum program, or a partial function

which returns the converted quantum program when called.

Good catch! Still a bit reminiscent of the old interface, so I've updated this doc string to

Program | partial: The Program object containing the converted quantum program, or a partial function of the `main` decorator.

(Technically the partial function returns the Program when called on the decorated function, but the language seems to imply an interface like we previously had)

rmshaffer · 2023-11-30T19:50:06Z

test/unit_tests/braket/experimental/autoqasm/test_api.py

+def _test_on_local_sim(program: aq.Program, inputs=None) -> None:
    device = LocalSimulator(backend=StateVectorSimulator())
-    task = device.run(program, shots=10)
+    task = device.run(program, shots=10, inputs=inputs or {})


[nit] any reason to not just have inputs={} as the default value?

Yes, Python behaves weirdly when you put a mutable object as a default in a function signature (if you mutate the object, it mutates the default for the function). (source)

I had no idea.

test/unit_tests/braket/experimental/autoqasm/test_api.py

ajberdy added 2 commits November 16, 2023 15:02

feat: remove *args from api decorators

621ae7c

draft: refactor main to build programs directly

ce413b5

ajberdy requested a review from a team as a code owner November 17, 2023 01:26

ajberdy added 17 commits November 16, 2023 17:27

lint

1b2ebcb

clean up main logic and add test

802c1f9

support free parameter expressions for qubit indices

71041f8

update test_types

39ed22d

update test_converters

c082e83

update test_devices

53ad41b

update test_gate_calibrations

a2193e7

update test_gate_decorator

264aaa5

update test_gate_definitions

79e3c08

update test_operators and lint

3b0807c

add closure test to py_if and py_assert tests

805b816

update test_parameters and add GateParameterType

d4d5430

update test_pragmas and lint

552c9d0

update test_program and lint

0d27fb0

update test_pulse and lint

d61dddb

update test_serialization_config

b5a7e5d

update testtranspiler

87f07c0

Base automatically changed from remove-args to feature/autoqasm November 17, 2023 22:18

ajberdy added 2 commits November 17, 2023 14:22

Merge branch 'feature/autoqasm' into build-program

9282b8d

post-merge lint

42a8b18

ajberdy added 2 commits November 17, 2023 14:37

lint

ef30424

add comment

e189311

laurencap reviewed Nov 18, 2023

View reviewed changes

ajberdy added 3 commits November 20, 2023 13:27

utilize function wrapper (#807)

1f76b13

utilize function wrapper

b8757da

keep args/kwargs in _convert_main for now

073a11b

rmshaffer reviewed Nov 28, 2023

View reviewed changes

ajberdy added 3 commits November 28, 2023 14:03

update example 1 text

6fb0644

remove vague todo

b555794

update example 3

18daf11

ajberdy added 5 commits November 28, 2023 15:40

update example 4

448fda4

update example 5

55e2e7b

update final example

cafe0ae

remove classmethod test

598af87

readd (modified) classmethod test for coverage

af6acec

laurencap self-assigned this Nov 29, 2023

laurencap reviewed Nov 29, 2023

View reviewed changes

remove xfailing test and update partial test in test_transpiler

b1590fe

laurencap reviewed Nov 29, 2023

View reviewed changes

examples/autoqasm/3_Iterative_phase_estimation.ipynb Outdated Show resolved Hide resolved

examples/autoqasm/3_Iterative_phase_estimation.ipynb Show resolved Hide resolved

examples/autoqasm/4_Native_programming.ipynb Show resolved Hide resolved

remove todo from example 3

b2bcd7e

rmshaffer approved these changes Nov 30, 2023

View reviewed changes

ajberdy added 2 commits November 30, 2023 12:08

add issue # to xfail

679bade

remove example todo and fix docstring

4406d4c

ajberdy requested a review from laurencap December 3, 2023 21:38

laurencap approved these changes Dec 4, 2023

View reviewed changes

ajberdy merged commit 2fff311 into feature/autoqasm Dec 4, 2023
23 checks passed

ajberdy deleted the build-program branch December 4, 2023 19:09

This was linked to issues Dec 4, 2023

AutoQASM: support for typed input parameters #813

Closed

Constraint @aq.main functions to OpenQASM inputs only. Unbound values become FreeParameters #805

Closed

rmshaffer mentioned this pull request Dec 4, 2023

AutoQASM: support for typed input parameters #813

Closed

ajberdy linked an issue Dec 4, 2023 that may be closed by this pull request

Remove secondary @aq.main call. A function decorated with @main is a Program #806

Closed

This was referenced Dec 4, 2023

Constraint @aq.main functions to OpenQASM inputs only. Unbound values become FreeParameters #805

Closed

Remove secondary @aq.main call. A function decorated with @main is a Program #806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: build autoqasm program directly from main decorator #804

feat: build autoqasm program directly from main decorator #804

ajberdy commented Nov 17, 2023 •

edited by rmshaffer

Loading

ajberdy commented Nov 17, 2023

laurencap left a comment

laurencap Nov 18, 2023

ajberdy Nov 18, 2023

laurencap Nov 20, 2023

rmshaffer Nov 28, 2023

ajberdy Nov 29, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

laurencap Nov 29, 2023

ajberdy Nov 29, 2023

laurencap Nov 29, 2023

ajberdy Nov 29, 2023

rmshaffer Nov 29, 2023

laurencap Nov 29, 2023

ajberdy Nov 29, 2023

laurencap Nov 29, 2023

ajberdy Nov 29, 2023

rmshaffer Nov 29, 2023

laurencap left a comment

rmshaffer commented Nov 30, 2023 •

edited

Loading

ajberdy commented Nov 30, 2023

rmshaffer left a comment

rmshaffer Nov 30, 2023

ajberdy Nov 30, 2023

rmshaffer Nov 30, 2023

ajberdy Nov 30, 2023 •

edited

Loading

rmshaffer Nov 30, 2023

ajberdy Nov 30, 2023

rmshaffer Dec 1, 2023


		assert main(4).to_ir() == expected
		assert main.make_bound_program(param_values={"n": 4}).to_ir() == expected

		Program \| partial: A callable
		which returns the converted quantum program when called.

feat: build autoqasm program directly from main decorator #804

feat: build autoqasm program directly from main decorator #804

Conversation

ajberdy commented Nov 17, 2023 • edited by rmshaffer Loading

Merge Checklist

General

Tests

ajberdy commented Nov 17, 2023

laurencap 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

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurencap left a comment

Choose a reason for hiding this comment

rmshaffer commented Nov 30, 2023 • edited Loading

ajberdy commented Nov 30, 2023

rmshaffer 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

ajberdy Nov 30, 2023 • edited Loading

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

ajberdy commented Nov 17, 2023 •

edited by rmshaffer

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

rmshaffer commented Nov 30, 2023 •

edited

Loading

ajberdy Nov 30, 2023 •

edited

Loading