Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Importing tomography with cvxpy installed fails #429

Closed
mtreinish opened this issue Jun 19, 2020 · 2 comments · Fixed by #431
Closed

Importing tomography with cvxpy installed fails #429

mtreinish opened this issue Jun 19, 2020 · 2 comments · Fixed by #431
Labels
bug Something isn't working

Comments

@mtreinish
Copy link
Collaborator

Informations

  • Qiskit Ignis version: 0.3.2
  • Python version: any
  • Operating system: any

What is the current behavior?

import qiskit.ignis.verification

raise an ImportErrorif cvxpy isn't installed:

ImportError: cannot import name 'cvxpy' from 'qiskit.ignis.verification.tomography.fitters.cvx_fit' (/home/mtreinish/git/qiskit/qiskit/.tox/docs/lib/python3.8/site-packages/qiskit/ignis/verification/tomography/fitters/cvx_fit.py)

It looks like an edge case was missed in #422. We don't have a ci job that doesn't install cvxpy so this is never tested and thus this slipped through

Steps to reproduce the problem

import qiskit.ignis.verification

What is the expected behavior?

You can import ignis without an optional dependency.

Suggested solutions

Fix the import in process tomography and add a test job.

@mtreinish mtreinish added the bug Something isn't working label Jun 19, 2020
mtreinish added a commit to mtreinish/qiskit that referenced this issue Jun 19, 2020
In the recent ignis 0.3.2 (and 0.3.1, but that had other bugs) release a
bug slipped in that crashes ignis if cvxpy is not installed. This is
causing the docs jobs to fail to build any documentation that ends up
importing process tomography (see qiskit-community/qiskit-ignis#429). This commit
explicitly adds cvxpy to the docs jobs to unblock them so we upload
complete documentation. An ignis bugfix release 0.3.3 will be out soon
to fix the ignis bug, but until then this will fix the hosted
documentation.
mtreinish added a commit to mtreinish/qiskit-ignis that referenced this issue Jun 19, 2020
This commit fixes an issue in the process tomography module. When cvxpy
was not installed the module would fail to import. That's because the
selection logic was reworked in qiskit-community#422 but never updated in process
tomography to reflect that change. Since no CI job runs without cvxpy
installed we never caught this edge case. This commit fixes the
underlying issue to rework the process tomography fitter selection logic
to mirror the changes to state tomography in qiskit-community#422 and then also try and
add ci coverage it removes cvxpy from the docs tox job. With warnings
set to fatal this should ensure we are always able to import everything
and build the docs if cvxpy is not installed.

Fixes qiskit-community#429
@gadial
Copy link
Contributor

gadial commented Jun 21, 2020

This was easy enough to fix (#433 ) - the crash resulted from Process tomography explicitly testing cvxpy (and if it is None, acting upon it), but this assignment was removed in #422 .

I was unable, however, to add a corresponding test; I tried something along the lines of

with unittest.mock.patch.dict(sys.modules, {'cvxpy': None}):
             del sys.modules['qiskit.ignis.verification.tomography']
             import qiskit.ignis.verification.tomography

but I couldn't get it to crash (while switching to a cvxpy-less Python worked, of course). @mtreinish , can you help me with this? What is the standard way in Python to test in the mocked presence of missing dependancies?

nonhermitian pushed a commit to Qiskit/qiskit-metapackage that referenced this issue Jun 22, 2020
In the recent ignis 0.3.2 (and 0.3.1, but that had other bugs) release a
bug slipped in that crashes ignis if cvxpy is not installed. This is
causing the docs jobs to fail to build any documentation that ends up
importing process tomography (see qiskit-community/qiskit-ignis#429). This commit
explicitly adds cvxpy to the docs jobs to unblock them so we upload
complete documentation. An ignis bugfix release 0.3.3 will be out soon
to fix the ignis bug, but until then this will fix the hosted
documentation.
@mtreinish
Copy link
Collaborator Author

@gadial mocking imports is not always straightforward because there is a multiple pieces of state to track and also module vs global scoping. You can see an example of what I did for a similar case in: https://github.com/Qiskit/qiskit-terra/pull/4296/files (in that case I wasn't trying to mock it away but replace a module with a mock).

That being said I already have a fix up in #431 which makes the docs job run without cvxpy which should catch this case in the future. But I also am going to push a new CI job that runs without cvxpy (and without matplotlib), it's the only way to reliably test the absence of a dependency across the board. We can mock it away in one place, but there might be unseen consequences somewhere else in the code (cvxpy's usage is pretty well isolated though).

mtreinish added a commit that referenced this issue Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy
was not installed the module would fail to import. That's because the
selection logic was reworked in #422 but never updated in process
tomography to reflect that change. Since no CI job runs without cvxpy
installed we never caught this edge case. This commit fixes the
underlying issue to rework the process tomography fitter selection logic
to mirror the changes to state tomography in #422 and then also try and
add ci coverage it removes cvxpy from the docs tox job. With warnings
set to fatal this should ensure we are always able to import everything
and build the docs if cvxpy is not installed.

Fixes #429
mtreinish added a commit to mtreinish/qiskit-ignis that referenced this issue Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy
was not installed the module would fail to import. That's because the
selection logic was reworked in qiskit-community#422 but never updated in process
tomography to reflect that change. Since no CI job runs without cvxpy
installed we never caught this edge case. This commit fixes the
underlying issue to rework the process tomography fitter selection logic
to mirror the changes to state tomography in qiskit-community#422 and then also try and
add ci coverage it removes cvxpy from the docs tox job. With warnings
set to fatal this should ensure we are always able to import everything
and build the docs if cvxpy is not installed.

Fixes qiskit-community#429

(cherry picked from commit 01e9070)
mtreinish added a commit to mtreinish/qiskit-ignis that referenced this issue Jun 22, 2020
This commit adds a new ci job for running ignis tests without any
optional dependencies. There are several optional dependencies which do
not not always get installed with ignis. They are needed to enable
optional features but shouldn't be required, we've had a slew of recent
bugs around accidentally requiring these optional dependencies (see
issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we
always install all optional dependencies in CI test jobs. By adding a
new job which explicitly installs the bare minimum we're emulating what
a user does when they install just ignis.

As part of this a missing dependency was added to the requirements list.
Ignis has a hard dependency on scikit learn for the measurement
discriminators, but this was never explicitly listed. This was never
caught because the test jobs were always installing it.
mtreinish added a commit to mtreinish/qiskit-ignis that referenced this issue Jun 22, 2020
This commit adds a new ci job for running ignis tests without any
optional dependencies. There are several optional dependencies which do
not not always get installed with ignis. They are needed to enable
optional features but shouldn't be required, we've had a slew of recent
bugs around accidentally requiring these optional dependencies (see
issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we
always install all optional dependencies in CI test jobs. By adding a
new job which explicitly installs the bare minimum we're emulating what
a user does when they install just ignis.

As part of this a missing dependency was added to the requirements list.
Ignis has a hard dependency on scikit learn for the measurement
discriminators, but this was never explicitly listed. This was never
caught because the test jobs were always installing it.
chriseclectic pushed a commit that referenced this issue Jun 22, 2020
This commit fixes an issue in the process tomography module. When cvxpy
was not installed the module would fail to import. That's because the
selection logic was reworked in #422 but never updated in process
tomography to reflect that change. Since no CI job runs without cvxpy
installed we never caught this edge case. This commit fixes the
underlying issue to rework the process tomography fitter selection logic
to mirror the changes to state tomography in #422 and then also try and
add ci coverage it removes cvxpy from the docs tox job. With warnings
set to fatal this should ensure we are always able to import everything
and build the docs if cvxpy is not installed.

Fixes #429

(cherry picked from commit 01e9070)
chriseclectic pushed a commit that referenced this issue Jun 22, 2020
…ies (#436)

* Add scikit-learn dependency and add CI job without optional deps

This commit adds a new ci job for running ignis tests without any
optional dependencies. There are several optional dependencies which do
not not always get installed with ignis. They are needed to enable
optional features but shouldn't be required, we've had a slew of recent
bugs around accidentally requiring these optional dependencies (see
issues #429, #422, and #312). None of these were caught in CI because we
always install all optional dependencies in CI test jobs. By adding a
new job which explicitly installs the bare minimum we're emulating what
a user does when they install just ignis.

As part of this a missing dependency was added to the requirements list.
Ignis has a hard dependency on scikit learn for the measurement
discriminators, but this was never explicitly listed. This was never
caught because the test jobs were always installing it.

* Don't install cvxopt in no-opt job

* Add job name
mtreinish added a commit to mtreinish/qiskit-ignis that referenced this issue Jun 22, 2020
…ies (qiskit-community#436)

* Add scikit-learn dependency and add CI job without optional deps

This commit adds a new ci job for running ignis tests without any
optional dependencies. There are several optional dependencies which do
not not always get installed with ignis. They are needed to enable
optional features but shouldn't be required, we've had a slew of recent
bugs around accidentally requiring these optional dependencies (see
issues qiskit-community#429, qiskit-community#422, and qiskit-community#312). None of these were caught in CI because we
always install all optional dependencies in CI test jobs. By adding a
new job which explicitly installs the bare minimum we're emulating what
a user does when they install just ignis.

As part of this a missing dependency was added to the requirements list.
Ignis has a hard dependency on scikit learn for the measurement
discriminators, but this was never explicitly listed. This was never
caught because the test jobs were always installing it.

* Don't install cvxopt in no-opt job

* Add job name

* Update tox.ini to point to remove master terra for stable branch

(cherry picked from commit f0b68e4)
chriseclectic pushed a commit that referenced this issue Jun 22, 2020
…ies (#436) (#439)

* Add scikit-learn dependency and add CI job without optional deps

This commit adds a new ci job for running ignis tests without any
optional dependencies. There are several optional dependencies which do
not not always get installed with ignis. They are needed to enable
optional features but shouldn't be required, we've had a slew of recent
bugs around accidentally requiring these optional dependencies (see
issues #429, #422, and #312). None of these were caught in CI because we
always install all optional dependencies in CI test jobs. By adding a
new job which explicitly installs the bare minimum we're emulating what
a user does when they install just ignis.

As part of this a missing dependency was added to the requirements list.
Ignis has a hard dependency on scikit learn for the measurement
discriminators, but this was never explicitly listed. This was never
caught because the test jobs were always installing it.

* Don't install cvxopt in no-opt job

* Add job name

* Update tox.ini to point to remove master terra for stable branch

(cherry picked from commit f0b68e4)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants