amazon-braket-sdk-python icon indicating copy to clipboard operation
amazon-braket-sdk-python copied to clipboard

feature: `add_verbatim_marker()` method to `BraketProgramContext` class for updated `Circuit.from_ir`

Open sim-eng-ii opened this issue 7 months ago • 1 comments

This PR and PR 285 were created to resolve Issue #972

Description of changes:

In this PR, the following change was made to support the updated Circuit.from_ir method:

  1. The class BraketProgramContext(AbstractProgramContext) was updated to include the method add_verbatim_marker(), it invokes:
    • Instruction(StartVerbatimBox(), target=[])
    • Instruction(EndVerbatimBox(), target=[])

These instructions are called when instances of VerbatimBoxStart and VerbatimBoxEnd are encountered.
add_verbatim_marker() method overrides the similarly named method from AbstractProgramContext, introduced in the amazon-braket-default-simulator-python repository in PR 285

This PR also includes a unit test for the changes to from_ir

Testing done:

A test function test_from_ir_with_verbatim_box() was added to test_circuit.py to verify that Circuit.from_ir correctly handles verbatim_box sections. The test was ran via command:

pytest test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_verbatim_box

and

pytest test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_verbatim_non_verbatim_instr

They both pass:


test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_mixed_verbatim_non_verbatim_instr 
[gw11] [100%] PASSED test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_mixed_verbatim_non_verbatim_instr 
====================== 1 passed in 5.89s =====================

test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_verbatim_box 
[gw11] [100%] PASSED test/unit_tests/braket/circuits/test_circuit.py::test_from_ir_with_verbatim_box 
====================== 1 passed in 6.11s ======================

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

Tests

  • [ x] I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • [ x] 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.

sim-eng-ii avatar May 30 '25 02:05 sim-eng-ii

I am currently completing the changes in my fork of amazon-braket-default-simulator-python. I will then open a PR, which should be reviewed alongside the one above. Thank you.

I have also successfully performed testing via tox -e unit-tests for amazon-braket-sdk-python

========== 2853 passed, 129 xfailed, 8 warnings in 141.91s (0:02:21) ===========
  unit-tests: OK (187.87=setup[41.88]+cmd[145.99] seconds)
  congratulations :) (187.96 seconds)

sim-eng-ii avatar May 30 '25 02:05 sim-eng-ii

hello @rmshaffer I will fix the code format issues

sim-eng-ii avatar Jun 30 '25 15:06 sim-eng-ii

The test will not pass until PR here is also confirmed and merged to main. The changes in the sdk work only if the amazon-default-simulator is updated

sim-eng-ii avatar Jun 30 '25 15:06 sim-eng-ii

The test will not pass until PR here is also confirmed and merged to main. The changes in the sdk work only if the amazon-default-simulator is updated

Got it, thanks! If you want to try to pass the CI in the meantime, before that PR is merged, I believe you could temporarily update this line in this PR: https://github.com/robotastray/amazon-braket-sdk-python/blob/main/tox.ini#L143 to point to your fork, i.e.

git+https://github.com/robotastray/amazon-braket-default-simulator-python.git

rmshaffer avatar Jun 30 '25 15:06 rmshaffer

hello @rmshaffer I will fix the code format issues

Yes, I see a lot of extraneous formatting changes in this PR - let us know if you need assistance with this.

You should just be able to run ruff check --fix from within the src folder to clean everything up there.

In the test folder, ruff is not actually run in CI. So you'll probably want to revert all of the formatting-specific changes that ruff made there, so that your PR doesn't have so many extra changes.

rmshaffer avatar Jun 30 '25 16:06 rmshaffer

The test will not pass until PR here is also confirmed and merged to main. The changes in the sdk work only if the amazon-default-simulator is updated

Got it, thanks! If you want to try to pass the CI in the meantime, before that PR is merged, I believe you could temporarily update this line in this PR: https://github.com/robotastray/amazon-braket-sdk-python/blob/main/tox.ini#L143 to point to your fork, i.e.

git+https://github.com/robotastray/amazon-braket-default-simulator-python.git

I have made the changes as you suggested

sim-eng-ii avatar Jun 30 '25 21:06 sim-eng-ii

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 100.00%. Comparing base (fa6f520) to head (17f1143). :warning: Report is 75 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1085   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          146       146           
  Lines         9872      9877    +5     
  Branches      1164      1164           
=========================================
+ Hits          9872      9877    +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jul 01 '25 00:07 codecov[bot]

Codecov Report

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

Project coverage is 99.98%. Comparing base (1b71ac8) to head (d9d6ddf). Report is 3 commits behind head on main.

Files with missing lines Patch % Lines src/braket/circuits/braket_program_context.py 88.88% 0 Missing and 1 partial ⚠️ Additional details and impacted files

☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. 🚀 New features to boost your workflow:

@rmshaffer is this failure about testing not being covered for the line of code highlighted?

sim-eng-ii avatar Jul 01 '25 16:07 sim-eng-ii

@robotastray it's indicating that both branches of the highlighted line are not being hit. To fix that error, you would want tests which cover all branching cases: enter the if block, enter the elif block, and skip both blocks.

rmshaffer avatar Jul 01 '25 17:07 rmshaffer

I have added additional testing and I have also updated the function add_verbatim_marker to make sure that if an invalid marker is added a TypeError is raised.

sim-eng-ii avatar Jul 03 '25 10:07 sim-eng-ii

I am currently looking at test/unit_tests/braket/experimental_capabilities/test_experimental_capabilities.py to see how my changes have impacted it. I will update once I have more info

sim-eng-ii avatar Jul 03 '25 14:07 sim-eng-ii

@rmshaffer the automated tests are checking for test_measureff_ccprx_from_ir. the function does not exists in test/unit_tests/braket/experimental_capabilities/iqm/test_iqm_experimental_capabilities.py

you can see the details in line 6376 on this page

sim-eng-ii avatar Jul 03 '25 15:07 sim-eng-ii

@rmshaffer the automated tests are checking for test_measureff_ccprx_from_ir. the function does not exists in test/unit_tests/braket/experimental_capabilities/iqm/test_iqm_experimental_capabilities.py

you can see the details in line 6376 on this page

@robotastray that test can be found in your fork here: https://github.com/robotastray/amazon-braket-sdk-python/blob/main/test/unit_tests/braket/experimental_capabilities/iqm/test_iqm_experimental_capabilities.py#L190-L206

It's a recently-added test in the upstream repo. I think this failure is expected - your changes are now parsing additional instructions out of the IR. I think you can just update the assertions there so that the test passes again with your changes. For example, replace

assert len(circuit_from_ir.instructions) == 3

with

assert len(circuit_from_ir.instructions) == 5

since it now contains the "start" and "end" verbatim box instructions.

rmshaffer avatar Jul 03 '25 17:07 rmshaffer

I updated test_iqm_experimental_capabilities.py to take into account the verbatim box. In addition to assert len(circuit_from_ir.instructions) == 5

I have also added few lines and updated others

assert circuit_from_ir.instructions[0].operator.name == "StartVerbatimBox"

assert circuit_from_ir.instructions[1].operator.name == "PRx"
assert isinstance(circuit_from_ir.instructions[2].operator, MeasureFF)
assert isinstance(circuit_from_ir.instructions[3].operator, CCPRx)
        
assert circuit_from_ir.instructions[4].operator.name == "EndVerbatimBox"

finally the indces have been updated on lines 200 and 205 to reflect the changes above

sim-eng-ii avatar Jul 03 '25 19:07 sim-eng-ii

@rmshaffer I updated the code in test_circuit.py and braket_program_context.py to make sure it is inline with the updates in #285

sim-eng-ii avatar Aug 16 '25 22:08 sim-eng-ii

Great work!

speller26 avatar Aug 21 '25 22:08 speller26