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

Avoid potential import cycles from Aer and IBMQ #5532

Closed
mtreinish opened this issue Dec 15, 2020 · 1 comment · Fixed by #5619
Closed

Avoid potential import cycles from Aer and IBMQ #5532

mtreinish opened this issue Dec 15, 2020 · 1 comment · Fixed by #5619
Labels
type: enhancement It's working, but needs polishing

Comments

@mtreinish
Copy link
Member

What is the expected enhancement?

Right now the effort to remove namespace packaging in #5089 is blocked on the modules level imports of Aer and IBMQ. This is because this causes an import cycle when those factory objects are no longer in the qiskit namespace. Basically, qiskit_aer imports qiskit whichi imports qiskit_aer.Aer which can't be resolved by python. To move forward on this we need to make these objects lazily load qiskit.providers.aer/qiskit_aer and qiskit.providers.ibmq/qiskit_ibmq_provider. The easiest way to handle this is to wait for python 3.7 to be the minimum python version and then we can just use a module level __getattr__ and import when Aer or IBMQ are accessed. But that is likely 6+ months away since 0.17.0 will start the deprecation timer on Python 3.6 support. In the meantime we should be able do something like:

class _AerWrapper:

    def __init__(self):
       self.aer = None
       
    def __getattr__(self, attr):
       if self._aer is None:
           try:
                from qiskit.providers.aer import Aer
            except ImportError:
                raise ImportError("useful message")
            self.aer = Aer
      return getattr(self.aer, attr)

(note this is completely untested, just my rough idea).

This should hopefully enable us to remove the import cycle at the module level. It would also have a noticeable benefit for #5100 as we're not unconditionally importing ibmq and aer all the time.

@mtreinish mtreinish added the type: enhancement It's working, but needs polishing label Dec 15, 2020
@mtreinish
Copy link
Member Author

The other thing that might complicate this is qiskit.__qiskit_version__ which also imports the elements version and is run at the module level. If it's an issue we should hopefully be able to come up with a similar hack for it, it's just a bit more involved because qiskit.__qiskit_version__ is a dict so when people use that they're actually only doing a module level getattr so we might be stuck until python 3.7 is the minimum version.

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jan 13, 2021
This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to
be lazy loading instances of lazy loading wrapper classes. The intent
here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from
qiskit-terra, while this is safe while the packages share a shared
namespace we've been actively working to remove the use of namespace
packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular
dependency caused by re-exporting these attributes is blocking progress
on this. By using a lazy loading wrapper class we avoid an import type
circular dependency and opportunistically use qiskit-aer and/or
qiskit-ibmq-provider at runtime only if they're present after everything
is imported. This also may have some benefit for the overall import
performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer
and qiskit-ibmq-provider from the import path unless they're being used.
Although the presence of qiskit.__qiskit_version__ might prevent any
performance improvements as it still imports all the elements to get
version information (and will be tackled in a separate PR).

Fixes Qiskit#5532
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jan 13, 2021
This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to
be lazy loading instances of lazy loading wrapper classes. The intent
here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from
qiskit-terra, while this is safe while the packages share a shared
namespace we've been actively working to remove the use of namespace
packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular
dependency caused by re-exporting these attributes is blocking progress
on this. By using a lazy loading wrapper class we avoid an import type
circular dependency and opportunistically use qiskit-aer and/or
qiskit-ibmq-provider at runtime only if they're present after everything
is imported. This also may have some benefit for the overall import
performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer
and qiskit-ibmq-provider from the import path unless they're being used.
Although the presence of qiskit.__qiskit_version__ might prevent any
performance improvements as it still imports all the elements to get
version information (and will be tackled in a separate PR).

Fixes Qiskit#5532
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jan 13, 2021
This commit migrates the __qiskit_version__ module attribute away from
using a dict constructed at import time to being an instance of a lazy
loading class that will import the elements to get their version
information only at run time if needed. This will improve the overall
import performance and also reduce the possibility of introducing an
import cycle when we remove namespace packaging.

Related to Qiskit#5100 and Qiskit#5532
mtreinish added a commit that referenced this issue Feb 2, 2021
* Add a lazy loading wrapper class for __qiskit_version__

This commit migrates the __qiskit_version__ module attribute away from
using a dict constructed at import time to being an instance of a lazy
loading class that will import the elements to get their version
information only at run time if needed. This will improve the overall
import performance and also reduce the possibility of introducing an
import cycle when we remove namespace packaging.

Related to #5100 and #5532

* Rename qiskit.execute module to qiskit.execute_function

The qiskit.execute module has a name conflict with the re-exported
path to it's sole public function qiskit.execute(). Whether python uses
the module or the function is an import time race condition and by
making the element imports occur at runtime this switches the normal
evaluation to use the module instead of the function. However, this was
always a bug and could have been triggered in other ways too. To make
the imports deterministic either the function or the module needs to be
renamed and since the module is less commonly used than the function
that is renamed.

* Rename qiskit.compiler inner modules to avoid name conflict

The qiskit.compiler module has a name conflict with the re-exported
path to public functions qiskit.compiler. Whether python uses
the module or the function is an import time race condition, amd
could be triggered by subtle changes in import evaluation order. To make
the imports deterministic either the function or the module needs to be
renamed and since the module is less commonly used than the function
that is renamed.

* Fix docs build

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <[email protected]>
@mergify mergify bot closed this as completed in #5619 Feb 5, 2021
mergify bot pushed a commit that referenced this issue Feb 5, 2021
* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object

This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to
be lazy loading instances of lazy loading wrapper classes. The intent
here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from
qiskit-terra, while this is safe while the packages share a shared
namespace we've been actively working to remove the use of namespace
packaging (see #5089, #4767, and #559) and the circular
dependency caused by re-exporting these attributes is blocking progress
on this. By using a lazy loading wrapper class we avoid an import type
circular dependency and opportunistically use qiskit-aer and/or
qiskit-ibmq-provider at runtime only if they're present after everything
is imported. This also may have some benefit for the overall import
performance of qiskit (being tracked in #5100) as it removes qiskit-aer
and qiskit-ibmq-provider from the import path unless they're being used.
Although the presence of qiskit.__qiskit_version__ might prevent any
performance improvements as it still imports all the elements to get
version information (and will be tackled in a separate PR).

Fixes #5532

* Fix lint

* Fix test lint

* DNM: test with ignis patch

* Revert "DNM: test with ignis patch"

This reverts commit ac9611c.

* Use ignis from source for tutorial job

* Update release note to be more clear
ElePT pushed a commit to ElePT/qiskit that referenced this issue Jun 27, 2023
* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object

This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to
be lazy loading instances of lazy loading wrapper classes. The intent
here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from
qiskit-terra, while this is safe while the packages share a shared
namespace we've been actively working to remove the use of namespace
packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular
dependency caused by re-exporting these attributes is blocking progress
on this. By using a lazy loading wrapper class we avoid an import type
circular dependency and opportunistically use qiskit-aer and/or
qiskit-ibmq-provider at runtime only if they're present after everything
is imported. This also may have some benefit for the overall import
performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer
and qiskit-ibmq-provider from the import path unless they're being used.
Although the presence of qiskit.__qiskit_version__ might prevent any
performance improvements as it still imports all the elements to get
version information (and will be tackled in a separate PR).

Fixes Qiskit#5532

* Fix lint

* Fix test lint

* DNM: test with ignis patch

* Revert "DNM: test with ignis patch"

This reverts commit ac9611c.

* Use ignis from source for tutorial job

* Update release note to be more clear
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this issue Jul 17, 2023
…it#5619)

* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object

This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to
be lazy loading instances of lazy loading wrapper classes. The intent
here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from
qiskit-terra, while this is safe while the packages share a shared
namespace we've been actively working to remove the use of namespace
packaging (see Qiskit/qiskit#5089, Qiskit/qiskit#4767, and Qiskit/qiskit#559) and the circular
dependency caused by re-exporting these attributes is blocking progress
on this. By using a lazy loading wrapper class we avoid an import type
circular dependency and opportunistically use qiskit-aer and/or
qiskit-ibmq-provider at runtime only if they're present after everything
is imported. This also may have some benefit for the overall import
performance of qiskit (being tracked in Qiskit/qiskit#5100) as it removes qiskit-aer
and qiskit-ibmq-provider from the import path unless they're being used.
Although the presence of qiskit.__qiskit_version__ might prevent any
performance improvements as it still imports all the elements to get
version information (and will be tackled in a separate PR).

Fixes Qiskit/qiskit#5532

* Fix lint

* Fix test lint

* DNM: test with ignis patch

* Revert "DNM: test with ignis patch"

This reverts commit ac9611c3ace30d101a70bda06e1987df14662182.

* Use ignis from source for tutorial job

* Update release note to be more clear
ElePT pushed a commit to ElePT/qiskit-ibm-provider that referenced this issue Oct 9, 2023
…t#5620)

* Add a lazy loading wrapper class for __qiskit_version__

This commit migrates the __qiskit_version__ module attribute away from
using a dict constructed at import time to being an instance of a lazy
loading class that will import the elements to get their version
information only at run time if needed. This will improve the overall
import performance and also reduce the possibility of introducing an
import cycle when we remove namespace packaging.

Related to Qiskit/qiskit#5100 and Qiskit/qiskit#5532

* Rename qiskit.execute module to qiskit.execute_function

The qiskit.execute module has a name conflict with the re-exported
path to it's sole public function qiskit.execute(). Whether python uses
the module or the function is an import time race condition and by
making the element imports occur at runtime this switches the normal
evaluation to use the module instead of the function. However, this was
always a bug and could have been triggered in other ways too. To make
the imports deterministic either the function or the module needs to be
renamed and since the module is less commonly used than the function
that is renamed.

* Rename qiskit.compiler inner modules to avoid name conflict

The qiskit.compiler module has a name conflict with the re-exported
path to public functions qiskit.compiler. Whether python uses
the module or the function is an import time race condition, amd
could be triggered by subtle changes in import evaluation order. To make
the imports deterministic either the function or the module needs to be
renamed and since the module is less commonly used than the function
that is renamed.

* Fix docs build

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement It's working, but needs polishing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant