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

Test default backend loading #1425

Closed
wants to merge 3 commits into from
Closed

Test default backend loading #1425

wants to merge 3 commits into from

Conversation

alecandido
Copy link
Member

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

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@alecandido alecandido marked this pull request as ready for review August 21, 2024 16:27
@alecandido alecandido linked an issue Aug 21, 2024 that may be closed by this pull request
@alecandido
Copy link
Member Author

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
Copy link
Member Author

alecandido commented Aug 21, 2024

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
Copy link
Member Author

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
Copy link
Member Author

The problem is slightly more complicated than expected: two different errors were raised

except ImportError as e:
# pylint: disable=unsupported-membership-test
if provider not in e.msg:
raise e
raise_error(
MissingBackend,

(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
Copy link
Member Author

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
Copy link
Member Author

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...

@alecandido alecandido changed the base branch from master to only-fix-1424 August 28, 2024 12:40
@alecandido alecandido added the bug Something isn't working label Aug 28, 2024
@alecandido alecandido marked this pull request as draft August 28, 2024 12:42
@alecandido alecandido changed the title Fix default backend loading Test default backend loading Aug 28, 2024
@BrunoLiegiBastonLiegi
Copy link
Contributor

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

@renatomello renatomello deleted the branch only-fix-1424 October 17, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GlobalBackend() fails if qibojit isn't available
3 participants