-
Notifications
You must be signed in to change notification settings - Fork 370
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
Move Aer to its own package #1526
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it works right to me, and both git and the build process seem to correctly locate all files. I couldn't find any instances of qiskit.providers.aer
(other than in the release notes) too, and the changes look mostly easy and mechanical.
I had a couple of comments about stuff that's orthogonal to this PR and doesn't need to be in scope, and one about version requirements on Terra to support the swap.
setup.py
Outdated
if not hasattr(setuptools, | ||
'find_namespace_packages') or not inspect.ismethod( | ||
setuptools.find_namespace_packages): | ||
print("Your setuptools version:'{}' does not support PEP 420 " | ||
"(find_namespace_packages). Upgrade it to version >='40.1.0' and " | ||
"repeat install.".format(setuptools.__version__)) | ||
sys.exit(1) | ||
|
||
VERSION_PATH = os.path.join(os.path.dirname(__file__), | ||
"qiskit", "providers", "aer", "VERSION.txt") | ||
"qiskit_aer", "VERSION.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to pair this with a version bump in the required version of Terra? The current requirement is 0.19.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can. We'll need to wait until terra 0.21.0 to do this since we'll need Qiskit/qiskit#5089 with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I just made a PR that increased the version requirement to 0.20 with a release note, so that'll want updating if we make this change.
d717552
to
b0189c9
Compare
Namespace packages are constant source of problems for users. The python packaging ecosystem around splitting packages across namespaces is fragile at the best of times and can often leave a you with an environment that isn't recoverable (especially when mixing install methods). There is also a performance hit whenever there is a piece of the namespace we allow external packages to extend since it requires doing a full python path search which can be slow depending on the backing I/O and the number of paths in sys.path for an environment. This commit addresses the piece from the aer perspective by moving qiskit.providers.aer to it's own package and namespace 'qiskit_aer'. This is coupled with a custom namespace module that implements a custom import loader at the root of the namespace. This has 2 advantages it removes the use of namespace packages so the fragility and performance impact are fixed since every element will be renamed to use 'qiskit_' instead of 'qiskit.' (aer being the only one remaining). This is the second attempt at doing this namespace rename, the first failed attempt in Qiskit#840 was trying to migrate all the former qiskit elements at the same time and was too difficult to migrate everything at once without causing breaking api changes. This commit is mechanically the same (renaming qiskit.providers.aer -> qiskit_aer), but how it integrates with qiskit-terra is changed. With Qiskit/qiskit#5089 qiskit-terra, the package which owns the 'qiskit.*' namespace, adds a metaimporter that will redirect imports from qiskit.providers.aer to qiskit_aer. This means before we can merge this commit Qiskit/qiskit#5089 will need to be released first to ensure that for backwards compatibility existing users will be able to access aer from the old namespace. Fixes Qiskit/qiskit#559
b0189c9
to
ec49a11
Compare
This reverts commit 4fa5bc9.
tox.ini
Outdated
@@ -4,11 +4,13 @@ envlist = py37, py38, py39, py310, lint | |||
skipsdist = True | |||
|
|||
[testenv] | |||
usedevelop=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? When I take it, docs test passed some errors (but finally failed with sphinx errors...)
setup.py
Outdated
@@ -112,7 +104,7 @@ def install_needed_req(import_name, package_name=None, min_version=None, max_ver | |||
setup( | |||
name=PACKAGE_NAME, | |||
version=VERSION, | |||
packages=setuptools.find_namespace_packages(include=['qiskit.*']), | |||
packages=setuptools.find_packages(include=['qiskit_aer.*']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that this copies packages in qiskit_aer/
but not files in qiskit
(qiskit_aer/__init__.py
, qiskit_aer/aererror.py
, qiskit_aer/aerprovider.py
and qiskit_aer/version.py
).
This commit fixes some issues in the new package configuration to tweak how find_package() call to ensure it is finding the full list of modules for the new qiskit_aer package and that things are installed correctly in CI. After this all the code should be installed with the package and the only remaining step is fixing how we run tests to avoid a conflict between $CWD/qiskit_aer and the installed qiskit_aer package. Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
I think this is finally good to go! Thanks @hhorii for doing last minute testing and fixes to get this working. |
In the recently merged Qiskit#1526 we moved the qiskit-aer package to move it's python namespace from qiskit.providers.aer to qiskit_aer. However, in that PR a release note was missing. This commit adds the missing release note to document the upgrade implications about the change. We should also mention this in the release notes prelude section, but that will occur in the release PR.
* Add release note for namespace migration In the recently merged #1526 we moved the qiskit-aer package to move it's python namespace from qiskit.providers.aer to qiskit_aer. However, in that PR a release note was missing. This commit adds the missing release note to document the upgrade implications about the change. We should also mention this in the release notes prelude section, but that will occur in the release PR. * Apply suggestions from code review Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake@binhbar.com>
Update test imports for qiskit-aer 0.11 See Qiskit/qiskit-aer#1526
* Backend timing helper for experiments (#860) Added `BackendTiming` class with methods for helping round delay and pulse durations to match backend timing constraints. Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update test imports for qiskit-aer 0.11 (#913) Update test imports for qiskit-aer 0.11 See Qiskit/qiskit-aer#1526 Co-authored-by: Will Shanks <willshanks@us.ibm.com> Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Update test imports for qiskit-aer 0.11 See Qiskit/qiskit-aer#1526
Update test imports for qiskit-aer 0.11 See Qiskit/qiskit-aer#1526
Summary
Namespace packages are constant source of problems for users. The python
packaging ecosystem around splitting packages across namespaces is
fragile at the best of times and can often leave a you with an
environment that isn't recoverable (especially when mixing install
methods). There is also a performance hit whenever there is a piece of
the namespace we allow external packages to extend since it requires
doing a full python path search which can be slow depending on the
backing I/O and the number of paths in sys.path for an environment. This
commit addresses the piece from the aer perspective by moving
qiskit.providers.aer to it's own package and namespace 'qiskit_aer'.
This is coupled with a custom namespace module that implements a
custom import loader at the root of the namespace. This has 2 advantages
it removes the use of namespace packages so the fragility and
performance impact are fixed since every element will be renamed to use
'qiskit_' instead of 'qiskit.' (aer being the only one remaining).
This is the second attempt at doing this namespace rename, the first
failed attempt in #840 was trying to migrate all the former qiskit
elements at the same time and was too difficult to migrate everything at
once without causing breaking api changes. This commit is mechanically
the same (renaming qiskit.providers.aer -> qiskit_aer), but how it
integrates with qiskit-terra is changed. With Qiskit/qiskit#5089
qiskit-terra, the
package which owns the 'qiskit.*' namespace, adds a metaimporter that
will redirect imports from qiskit.providers.aer to qiskit_aer. This
means before we can merge this commit Qiskit/qiskit#5089 will need
to be released first to ensure that for backwards compatibility existing
users will be able to access aer from the old namespace.
Details and comments
Depends on: Qiskit/qiskit#5089
Fixes Qiskit/qiskit#559