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

Tests #11

Merged
merged 35 commits into from
Mar 3, 2024
Merged

Tests #11

merged 35 commits into from
Mar 3, 2024

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

Use secrets to make tests run.

@alecandido
Copy link
Member

I know that debugging workflows is frustrating, but helpful commit messages are always useful...

Moreover, in principle it could be possible to even debug locally (unofficial):
https://github.com/nektos/act
(to be fair: I've never tried myself, but it's plenty of stars...)

@alecandido
Copy link
Member

Btw, the one failing was the docs workflow (that should not be involved here). The one being fixed is not even running, because of the missing secret...

@scarrazza
Copy link
Member

@alecandido @BrunoLiegiBastonLiegi could you please have a look at the CI please?

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

@alecandido @BrunoLiegiBastonLiegi could you please have a look at the CI please?

mmmh I am trying to run the tests locally but qibo-client hangs there forever, or at least until I waited...
I also tried changing back the port to 80, but it's the same

[2024-03-02 15:38:46,829] INFO: Post new circuit on the server
[2024-03-02 15:38:47,113] INFO: Job posted on server with pid 15b51361ab064dc5a81be36218bee7ca
[2024-03-02 15:38:47,113] INFO: Check results every 2 seconds ...

@alecandido
Copy link
Member

alecandido commented Mar 2, 2024

The correct port is definitely 8080 (despite 80 being the default in qibo-client). Though the concept of "upgrade" of qibo-client is pretty peculiar...

E       AssertionError: Local Qibo package version does not match the server one, please upgrade: 0.2.5-> 0.2.4

(btw, I don't have a valid token to test, but the version check seems to happen before that)

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@scarrazza
Copy link
Member

@alecandido @BrunoLiegiBastonLiegi please have another look, this is probably ready.

We have already in qibo-client issues related to the verbosity of bad configurations / errors, but v0.0.3 should be considered as an alpha release.

@scarrazza scarrazza marked this pull request as ready for review March 3, 2024 02:19
@BrunoLiegiBastonLiegi
Copy link
Contributor Author

BrunoLiegiBastonLiegi commented Mar 3, 2024

This looks ready to me. Should we add in the documentation a reminder about setting port 8080 with qibo-client or is this something that we are planning to change in the future anyway?

One other thing that I think we should do is making qiskit an optional dependency. By default this will only ship with qibo-client, then the user can install whatever he needs
pip install qibocloud[qiskit,ionq,ecc...]

@alecandido
Copy link
Member

This looks ready to me. Should we add in the documentation a reminder about setting port 8080 with qibo-client or is this something that we are planning to change in the future anyway?

I'd say that there is no point in having the wrong port configured in qibo-client.
If a port is specified, let's make it the correct one.

@@ -18,6 +20,7 @@ class QiboClientBackend(NumpyBackend):

def __init__(self, token, provider=None, platform=None):
super().__init__()
os.environ.setdefault("QRCCLUSTER_PORT", "8080")
Copy link
Member

Choose a reason for hiding this comment

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

That's also fine, for the time being. But the proper place will be:
https://github.com/qiboteam/qibo-client/blob/4e70c049bb66475d9af468fe3e9611d75fd5e5fb/src/qibo_client/tii.py#L6
since providers are defined there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok I see, makes sense. By the way, for some reason if you set the env variable inside the backend it won't be set correctly inside of the test, but I am not sure why...

Copy link
Member

@alecandido alecandido Mar 3, 2024

Choose a reason for hiding this comment

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

I'm now checking it

Copy link
Member

Choose a reason for hiding this comment

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

Error found: the environment is probed top-level, both in qibo-cloud-backends and qibo-client, thus it is determined at import time, and insensitive to updates.

This is a problem in different ways, I will file one issue per repo describing it. The hotfix is in c922218, and just reverts what you did.

pyproject.toml Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

alecandido commented Mar 3, 2024

We have already in qibo-client issues related to the verbosity of bad configurations / errors, but v0.0.3 should be considered as an alpha release.

I guess this was related to qiboteam/qibo-client#21

Assign default value for tokens
@scarrazza
Copy link
Member

My suggestion is to keep the code here port independent or at least do not hard code 8080 anywhere. We are planning to move from 8080 to 80 soon in this month.

@alecandido
Copy link
Member

alecandido commented Mar 3, 2024

My suggestion is to keep the code here port independent or at least do not hard code 8080 anywhere. We are planning to move from 8080 to 80 soon in this month.

We could move the port setting to the test (I thought it was already, but I was wrong)

EDIT: I saw #11 (comment) only after writing, I actually moved to the workflow

@scarrazza
Copy link
Member

Perfect @alecandido, thanks!

@scarrazza scarrazza self-requested a review March 3, 2024 13:18
Since it is now allowed by qibo-client v0.0.3
@scarrazza scarrazza merged commit e173e5e into main Mar 3, 2024
12 checks passed
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.

None yet

4 participants