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

MetaBackend #7

Merged
merged 42 commits into from
Jul 1, 2024
Merged

MetaBackend #7

merged 42 commits into from
Jul 1, 2024

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

This PR implements a MetaBackend to load the qibocloud backend and list the available workers.

available_backends = {}
for worker in WORKERS:
try:
MetaBackend.load(worker=worker, token="")
Copy link
Member

Choose a reason for hiding this comment

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

Is .load enough here to grant that you have a valid connection and you can submit circuits?
Or can you check that you can submit, without actually submitting anything?

Copy link
Contributor Author

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi Feb 27, 2024

Choose a reason for hiding this comment

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

No, indeed, I think here the token is missing. I am pretty sure that qiskit performs a connection test when a QickitClientBackend is created. I don't know whether qibo-client supports a similar pinging mechanism, but that it is what we would need. However, then you would still need to have available the token inside list_available, should we ask the user to set up a global TOKEN shell variable that we will then read inside here?

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi marked this pull request as ready for review April 23, 2024 11:03
@scarrazza
Copy link
Member

@BrunoLiegiBastonLiegi the qibo-client should be working now, could you please check why tests are failing?

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Mmmmh weird, I am still getting the 404 error with qibo-client:

requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://cloud.qibo.science/run_circuit/

even running the example in the readme gives me the 404

import qibo
import qibo_client

# create the circuit you want to run
circuit = qibo.models.QFT(5)

# authenticate to server through the client instance
token = "xxxxxxx"
client = qibo_client.TII(token)

# run the circuit
result = client.run_circuit(circuit, nshots=1000, device="sim")

Furthermore, now the QiskitClientBackend is failing as well because apparently the ibmq_qasm_simulator is not available anymore as a backend on the IBMQ endpoint:

qiskit.providers.exceptions.QiskitBackendNotFoundError: 'No backend matches the criteria'

maybe they changed the name, I am not sure, I will need to check.

@scarrazza
Copy link
Member

@BrunoLiegiBastonLiegi after updating the Qibo version, the issue with qibo-client is now fixed.
Could you please have another look at tests?

@alecandido
Copy link
Member

@BrunoLiegiBastonLiegi you could even trigger them periodically, like once per month. It would ensure things are being tested, and at the rate you wish, saving you the trouble to do it manually (and keeping track of the last time you did it).

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

You could run them manually on top of that, whenever you have a specific need.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Yeah I would do something like that, once a month seems reasonable. Then, I would remove the auto trigger, for tests at least, on push event, set a periodical one once a month and ideally each PR should run the tests at least once before being merged. Can you have a button in the PR to trigger the CI? I imagine the PR process to be something along the lines:

  • implement new features
  • test locally until everything works
  • trigger manually the CI tests
  • review
  • fix what's there to fix
  • trigger manually the CI tests one last time before merging

Alternatively, we could decouple the ibm-q tests that we know to be time consuming and expensive and apply all of the above to those alone.

Regarding the environment variables for the tokens, instead, I would like to propose a change of naming:

"QIBO_CLIENT_TII_TOKEN" --> "QIBO_CLIENT_TOKEN"
"IBMQ_TOKEN" --> "QISKIT_CLIENT_TOKEN"

as in my opinion, they should be coupled to the client backend rather than the provider/platform.

@alecandido
Copy link
Member

You can not have a PR button. Instead, what you can have is:

that you can use to trigger the workflow.

A real button to be displayed in the web UI is not available, but I guess you can get really close with the other options (especially the label).

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

/run_tests

@scarrazza
Copy link
Member

"QIBO_CLIENT_TII_TOKEN" --> "QIBO_CLIENT_TOKEN"
"IBMQ_TOKEN" --> "QISKIT_CLIENT_TOKEN"

I agree, in fact in the future we might have other labs linked to the qibo-client, therefore no need to create separate tokens.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

BrunoLiegiBastonLiegi commented Jun 29, 2024

/run_tests

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

@alecandido do you have any idea why /run_tests is not triggering the tests? Am I missing something?

@alecandido
Copy link
Member

@alecandido do you have any idea why /run_tests is not triggering the tests? Am I missing something?

The way I read this

if: contains(github.event.pull_request.labels.*.name, 'run-workflow') || github.event_name == 'push'

is: "run this job if the label contains run-workflow or if it is a push event".

In principle, your event has no label, and it should be an issue_comment event. So, the workflow should not get triggered.

Given that the label was unused, maybe you want to replace the condition on the label with the one on the comment (or just add the one about the comment).

@scarrazza
Copy link
Member

For the sake of symmetry with other repositories, may I suggest to use the label mechanism instead of comment?

@@ -55,12 +53,11 @@ jobs:
poe lint
poe lint-warnings
- name: Test
if: contains(github.event.comment.body, '/run_tests') # check if the comment calls the tests execution
Copy link
Member

Choose a reason for hiding this comment

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

But now you should replace this with the label query (and schedule, whenever there will be a schedule event).

@scarrazza
Copy link
Member

@BrunoLiegiBastonLiegi I would like to merge this PR asap and release.
Could you please double check if everything is in place?

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Yeah sorry I am traveling for a conference this week and my time is going to be limited. Anyway, the PR looks mostly ready to me: the tests now trigger only when the run-workflow label is added, what is missing is the periodical test in case we want to add it and change of the environment variables name.

@alecandido
Copy link
Member

Since there is no trigger on push: any longer, I agree with @BrunoLiegiBastonLiegi.

Eventually, I'd split some tests that could be executed on push: (even mocking) from the remote ones (to be executed with the current events). And add the scheduling.
But all of this could be done in separate PRs, so I'd just merge this one (which was targeted to support the MetaBackend mechanism, not to improve the testing one).

@scarrazza
Copy link
Member

Thanks @BrunoLiegiBastonLiegi and @alecandido. I will merge and release this repo now, please open the respective issues concerning the workflows.

@scarrazza scarrazza merged commit dfcd3ad into main Jul 1, 2024
12 checks passed
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

4 participants