search

qiboteam / qibo

A full-stack framework for quantum computing.
https://qibo.science
Apache License 2.0
294 stars 60 forks source link

Test default backend loading #1425

Closed alecandido closed 1 month ago

alecandido commented 2 months ago

There is an error mismatch, but this was undetected in the CI, because Qibojit is always available.

Checklist:

alecandido commented 2 months ago

The first commit introduces the failing test, the second one the fix.

@chmwzc: could you also check that is working for you locally? I'm pretty sure, because I managed to reproduce the error, but I learned that one check more is never wasted, and environments issues may be tricky...

alecandido commented 2 months ago

Ok, for some reason, I screwed another test. I will fix it, but the one I added is actually passing (and it was not in the commit in which it was introduced), so keep going with the review.

alecandido commented 2 months ago

I slightly changed the solution approach, providing a custom error.

The rationale is that from the point of view of set_backend() (the one whose test was failing) and similar functions, it makes sense to raise a ValueError: the user asked for something, and that something is not available, so it's a wrong value (despite the type being the correct one). The fact that this is happening through an import is hidden, so an ImportError would be more surprising.

At the same time, I didn't want to catch whatever ValueError in the world, since it could be unrelated to the failing import. And that's why the dedicated error.

alecandido commented 2 months ago

The problem is slightly more complicated than expected: two different errors were raised https://github.com/qiboteam/qibo/blob/079691034fda3005e92c50392ae7e6665d858cc2/src/qibo/backends/__init__.py#L230-L235 (as they are again, with MissingBackend)

This seems to have been almost purposeful. Or, at least, it is exploited in the tests. Indeed, the error received in #1424 is used to distinguish when the package providing the backend is missing, from when a dependency of that package was missing. In practice, when you create the default backend, you don't care why you can not import it: you can't, so you should step forward to the next one.

I'm not sure it's a great idea to distinguish the two cases in general, because you also don't care that much to distinguish the reason, as a user. So, you'd like to receive the same error in both cases, most probably (even losing the hint about the package installation). However, here's where the tests kick in: they need to distinguish, because they want to know where the dependencies are not available.

Well, we could even change the conftest.py to check for a MissingBackend, and drop the distinction above. But we should check there are no other side-effects, and change the error message. It's definitely going beyond the quickfix for the issue above, and it's another kind of improvement (not fixing a bug, but changing to user interface, since changing the error raised, with all possible downstream effects). I will open an issue for this, and take the time to solve it properly later on.

For the time being, the quickfix should work just for the issue described (catching both types of errors only during default backend creation).

alecandido commented 2 months ago

Apparently now there is a problem with the fixture, that is working on Mac, but making qibojit disappear on Linux and Windows 🤦

I will try to debug locally on Linux...

alecandido commented 2 months ago

It seems I pushed too much with the test, since now I have a problem in the CI (not on MacOS) I can not reproduce locally (neither on Linux nor on MacOS itself).

There is also a residual "earnest" issue to address, that I'm detecting even locally (about qulacs availability). That I will fix immediately.

I'm trying to debug the issue, but whenever the CI is involved it could be a pain...

BrunoLiegiBastonLiegi commented 2 months ago

I can confirm that this works for me as well, locally on linux.