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

Introduce braket_client #30

Merged
merged 121 commits into from
Nov 5, 2024
Merged

Conversation

mho291
Copy link
Contributor

@mho291 mho291 commented Jul 17, 2024

No description provided.

mho291 and others added 30 commits May 9, 2024 10:02
* initial aws_client commit

* remove .idea

* added amazon packages

* Added functions to replace qasm gates and verbatim box

* Modified qasm_convert_gates, added comments, removed topology argument

* change aws test to randomnized clifford like the others.

* added extras into installs and removed conditional classes

* Added transpiler (and relevant inputs for transpiler), modified method of adding verbatim box

* made poetry install all extras in unit_test.yml

* remove module checks

* updated tests

* Added two function. 1. Transpile qasm to qasm using Qiskit, 2. Transpile qibo to braket circuit. Modified execute_circuit to only include inputs for verbatim_circuits and nshots.

* Updated aws_client.py to sync with the latest aws_client.py residing in the "initial_setup" branch.

---------

Co-authored-by: TL231 <[email protected]>
…cuits. Shifted docstring under __init__ to the class.
Contains instructions on how to get Phase RX gate, ZNE get_noisy_circuit to work in the notebook.
Previously, the qibo measurement gates will also be wrapped within the verbatim box if `verbatim_circuit = True`, raising an error. 

Solution: Create a copy of the input qibo circuit without measurement gates, extract qubit indices of measurements gates. Then convert to Braket and wrap verbatim box if needed. Finally, add the measurement gates back to the Braket circuit.
…d qasm transpilation, added job monitoring, moved transpilation args from init to transpilation func
Update qibo version & braket-sdk version
@alecandido
Copy link
Member

@mho291 you should not use poetry lock to resolve the conflicts. But rather solve them manually, and then use poetry lock.
However, this is only relevant if you use poetry lock --no-update, since it will be aware of the current state. If you instead use poetry lock, it should recompute from scratch, so a convenient alternative becomes just deleting the conflicting poetry.lock with rm, and then run poetry lock to recreate it from scratch. In this case, it's truly impossible that the conflicts will stay (for the simple reason that they should not be recorded in any other place...)

For pyproject.toml, there are two areas to resolve:

However, here you're talking about pyproject.toml, not poetry.lock, which are definitely two separate files :)

my guess is we should add both instead of removing, since we need amazon-braket-sdk. Can I confirm if that's right?

Well, I can tell you that you should not do that, for the simple reason that the TOML file is like a dict, so you would have two equal keys - in the best case the loader will overwrite the former with the latter, in the worst it will just fail for the duplicated entry.

So, you have to choose. Which one to choose is a separate story. But the usage of extras is consistent in qibo-cloud-backends, and it should be recommended for any package providing multiple backends (since the user may need a single one, and reducing dependencies when possible is always making the installation and run more reliable).
I.e. you should only accept the optional, no reason to accept the one which is overwriting it.

For the poetry.lock file, will it suffice to accept the changes, except those that ask amazon-braket-sdk to be removed, using the web editor?

In any case, I don't see any residual conflict in the repo. So, it seems that you only have the conflicts and you never pushed them (which would be the correct thing to do).

Please, refrain from using the web editor: if you're unable to solve conflicts locally, that's a good reason to dig deeper. Not to propagate them to remote repo through a different path.

@mho291
Copy link
Contributor Author

mho291 commented Oct 15, 2024

Thanks @alecandido , I have a better picture now.

I noticed that we are 113 commits ahead of, 20 commits behind qiboteam/qibo-cloud-backends:main.

So what I'm doing (currently still in progress) is to update the changes in accordance to the 20 commits we're behind. I've also included the amazon-braket-sdk dependency inside the pyproject.toml file for this pull request. And then generate a new poetry.lock file from scratch. After that, I will push.

Thanks a lot!

@alecandido
Copy link
Member

alecandido commented Oct 15, 2024

So what I'm doing (currently still in progress) is to update the changes in accordance to the 20 commits we're behind. I've also included the amazon-braket-sdk dependency inside the pyproject.toml file for this pull request. And then generate a new poetry.lock file from scratch. After that, I will push.

To do this, I suggest to either do git rebase upstream main (which is my favorite option, but it may be a bit painful if you're not familiar with rebasing), or rather git merge upstream main.

And yes, this will generate conflicts, which in general you will need to solve manually. And that's also the case for pyproject.toml and poetry.lock (though, for the second one, the removal strategy is often a practical one - of course after pyproject.toml manual resolution).

@mho291
Copy link
Contributor Author

mho291 commented Oct 15, 2024

So what I'm doing (currently still in progress) is to update the changes in accordance to the 20 commits we're behind. I've also included the amazon-braket-sdk dependency inside the pyproject.toml file for this pull request. And then generate a new poetry.lock file from scratch. After that, I will push.

To do this, I suggest to either do git rebase upstream main (which is my favorite option, but it may be a bit painful if you're not familiar with rebasing), or rather git merge upstream main.

And yes, this will generate conflicts, which in general you will need to solve manually. And that's also the case for pyproject.toml and poetry.lock (though, for the second one, the removal strategy is often a practical one - of course after pyproject.toml manual resolution).

Super! Thanks so much for the guidance!

pyproject.toml Outdated
Comment on lines 51 to 52
qiskit_ibm_provider = "^0.11"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
qiskit_ibm_provider = "^0.11"
qiskit_ibm_provider = "^0.11"
amazon-braket-sdk = "^1.83.0"

if in the docs you are running code examples with BraketClient you probably need the amazon-braket-sdk here as well, otherwise the docs deployment will fail.

@alecandido
Copy link
Member

@BrunoLiegiBastonLiegi I dismissed your review since it happened a while ago, so it made sense in general to confirm the opinion is unchanged.

If you still believe that this is good to go, and you don't need to take a further look, feel free to approve it again.

@mho291
Copy link
Contributor Author

mho291 commented Oct 25, 2024

Hi! Wondering if we could expedite the merging of this. I have a few colleagues who will need to start using the cloud interface for their work in early November. Thank you!!

Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I've been a little busy lately. This is still looking good to me, can you please just add the token argument to the backend even though it's not used?



class BraketClientBackend(NumpyBackend):
def __init__(self, device=None, verbatim_circuit=False, verbosity=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, device=None, verbatim_circuit=False, verbosity=False):
def __init__(self, device=None, verbatim_circuit=False, verbosity=False, token: str=None):

For symmetry with the other backends can you add a token=None argument here, and raise a warning or an error if the user actually passes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to leave token argument in just for symmetry with other cloud backends, because Braket doesn't need the token argument. A user may be confused and wonder if there will be a feature for token in future. What do you think? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that it may create some confusion, but I ultimately think that it would be better to uniform all the backends under the input arguments perspective. It should be fine, as long as this is well documented and/or warnings are raised for unused arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. In that case, I'm thinking we could raise a warning:

import warnings
def __init__(self, device=None, verbatim_circuit=False, verbosity=False, token: str=None):
    if token:
        warnings.warn(
            f"Token is not needed for executing circuits on Amazon Braket devices.",
            category=UserWarning
        )

That way, the code can run even with token present. Do you think this is alright?

Copy link
Member

Choose a reason for hiding this comment

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

Once more: refrain from warnings if you're not directly writing an app (i.e. a CLI or a graphical app).

Choose your favorite among:

  • ignoring
  • raise an error
  • return some marker (possibly together with other values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might choose ignore because raise_error will stop the code from execution. But let me try all the options out on my end. Thanks a lot!

Comment on lines 41 to 42
if token:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if token:
pass

thanks, we could even drop this if we decide to ignore it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sure!

@BrunoLiegiBastonLiegi
Copy link
Contributor

BrunoLiegiBastonLiegi commented Nov 4, 2024

Thanks @mho291, I just checked that the braket tests are passing and they seem to (the errors come from the ibmq account in the secrets apparently). Personally I would merge this now as it looks good enough to me for the moment. We could iteratively improve on this in future with other PRs.
Thus, unless, there is any objection, I would merge this.

@mho291
Copy link
Contributor Author

mho291 commented Nov 5, 2024

Hi @BrunoLiegiBastonLiegi , sounds great! Thanks so much for guiding and reviewing. Once merged, I will also create an issue with the following to remind us of what we discussed earlier.

Possibility of qibo -> qasm -> qiskit -> braket conversion.

  • To do that, implement a function, either inside this backend or outside but called inside the execute_circuit here
  • The function will build a qiskit circuit starting from the qibo generated QASM + some custom instructions through the qasm2.loads and overriding the qiskit.from_qasm
  • Finally, make use of the existing qiskit -> braket conversion layer to create the braket circuit and send it through

The qasm->qiskit has problems:

  • Qiskit uses r to represent the phase RX gate.
  • Changing qibo's PRX qasm representation from prx to r doesn't work -- qasm loader for qiskit doesn't recognise it because it's not in qiskit's LEGACY_CUSTOM_INSTRUCTIONS. (See comment)
  • Adding MY_CUSTOM_INSTRUCTIONS to LEGACY_CUSTOM_INSTRUCTIONS also doesn't work. (See comment)

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi merged commit d4e051b into qiboteam:main Nov 5, 2024
1 of 7 checks passed
@mho291
Copy link
Contributor Author

mho291 commented Nov 13, 2024

Hi @BrunoLiegiBastonLiegi , since Braket client has been merged, is there a way to use it before the release? Thanks! :)

@BrunoLiegiBastonLiegi
Copy link
Contributor

You just clone the repo (main branch) and do pip install . (or poetry install).
Alternatively, without cloning: pip install git+https://github.com/qiboteam/qibo-cloud-backends.

@mho291
Copy link
Contributor Author

mho291 commented Nov 13, 2024

You just clone the repo (main branch) and do pip install . (or poetry install). Alternatively, without cloning: pip install git+https://github.com/qiboteam/qibo-cloud-backends.

Thanks very much!!

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.

7 participants