Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use relative import for _accelerate #12546

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

garrison
Copy link
Member

@garrison garrison commented Jun 11, 2024

Summary

I don't understand the python import mechanism enough to understand why, but this PR fixes some problems for me with importing from Rust code when on Fedora or RHEL.

Details and comments

If one tries to run tox -epy from a dependent of Qiskit on Fedora or RHEL, there will be lots of errors about qiskit._accelerate. For the sake of example, we will use the circuit-knitting-toolbox as an example of a Qiskit dependent, but I've also been able to reproduce this with qiskit-experiments.

Here is a Dockerfile that tries to do this using the current latest commit on Qiskit main:

FROM fedora:40
RUN dnf install -y pip git rust cargo
RUN pip install tox
RUN git clone https://github.com/Qiskit-Extensions/circuit-knitting-toolbox.git
WORKDIR /circuit-knitting-toolbox
RUN echo '[tool.hatch.metadata]' >> pyproject.toml && \
    echo 'allow-direct-references = true' >> pyproject.toml && \
    sed -i 's/qiskit>=1.0.0, <2.0/qiskit @ git+https:\/\/github.com\/Qiskit\/qiskit.git@03d107e1f68f2a47ffdcf5599f906120e4efba63/' pyproject.toml && \
    cat pyproject.toml
RUN tox -epy

When one attempts to build this image using docker build -f Dockerfile.qiskit-main ., the following error is raised repeatedly:

    from qiskit.circuit import (
.tox/py/lib64/python3.12/site-packages/qiskit/__init__.py:62: in <module>
    sys.modules["qiskit._accelerate.circuit"] = qiskit._accelerate.circuit
E   AttributeError: partially initialized module 'qiskit' has no attribute '_accelerate' (most likely due to a circular import)

If one instead uses a Dockerfile that points to this fork (same as above except one line),

FROM fedora:40
RUN dnf install -y pip git rust cargo
RUN pip install tox
RUN git clone https://github.com/Qiskit-Extensions/circuit-knitting-toolbox.git
WORKDIR /circuit-knitting-toolbox
RUN echo '[tool.hatch.metadata]' >> pyproject.toml && \
    echo 'allow-direct-references = true' >> pyproject.toml && \
    sed -i 's/qiskit>=1.0.0, <2.0/qiskit @ git+https:\/\/github.com\/garrison\/qiskit.git@relative-toplevel-imports/' pyproject.toml && \
    cat pyproject.toml
RUN tox -epy

then the errors are very different:

ImportError while importing test module '/circuit-knitting-toolbox/test/utils/test_transforms.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib64/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/utils/test_transforms.py:17: in <module>
    from qiskit.circuit import (
.tox/py/lib64/python3.12/site-packages/qiskit/__init__.py:90: in <module>
    from qiskit.circuit import ClassicalRegister
.tox/py/lib64/python3.12/site-packages/qiskit/circuit/__init__.py:1213: in <module>
    from . import _utils
.tox/py/lib64/python3.12/site-packages/qiskit/circuit/_utils.py:22: in <module>
    from .parametervector import ParameterVectorElement
.tox/py/lib64/python3.12/site-packages/qiskit/circuit/parametervector.py:17: in <module>
    from .parameter import Parameter
.tox/py/lib64/python3.12/site-packages/qiskit/circuit/parameter.py:20: in <module>
    import symengine
.tox/py/lib/python3.12/site-packages/symengine/__init__.py:12: in <module>
    import symengine.lib.symengine_wrapper as wrapper
E   ModuleNotFoundError: No module named 'symengine.lib.symengine_wrapper'

which is a different, known, issue (but I am having a hard time finding a link at the moment). EDIT: I believe it may be related to symengine/symengine-wheels#16.

Thus, I propose that the current PR is merged to get Qiskit one step closer to working properly on Fedora and RHEL.

@garrison garrison marked this pull request as ready for review June 11, 2024 15:24
@garrison garrison requested a review from a team as a code owner June 11, 2024 15:24
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this, especially if it's helping you. Black can probably join together some of the lines now, since they're shorter.

import qiskit._accelerate
from . import _accelerate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this line, we probably don't actually need to change qiskit._accelerate into _accelerate, because this line still causes _accelerate to be a name that's defined on the qiskit object.

But also, I don't mind at all.

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9468397965

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • 18 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.585%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 92.62%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 9455662900: -0.02%
Covered Lines: 62509
Relevant Lines: 69776

💛 - Coveralls

@garrison
Copy link
Member Author

Black can probably join together some of the lines now, since they're shorter.

Sorry, I will fix this. I fixed it once, but I messed it up again when I when to rebase to the latest main and had to resolve a conflict.

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9469996422

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.596%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/parameterexpression.py 2 96.54%
crates/qasm2/src/lex.rs 11 91.6%
Totals Coverage Status
Change from base Build 9455662900: -0.01%
Covered Lines: 62518
Relevant Lines: 69778

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 12, 2024

Pull Request Test Coverage Report for Build 9475648590

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • 40 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.05%) to 89.558%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/parameterexpression.py 2 96.54%
crates/qasm2/src/lex.rs 8 92.62%
crates/qasm2/src/parse.rs 30 95.77%
Totals Coverage Status
Change from base Build 9455662900: -0.05%
Covered Lines: 62492
Relevant Lines: 69778

💛 - Coveralls

@jakelishman jakelishman added this pull request to the merge queue Jun 12, 2024
Merged via the queue into Qiskit:main with commit 287e86c Jun 12, 2024
15 checks passed
@garrison garrison deleted the relative-toplevel-imports branch June 12, 2024 11:41
@ElePT ElePT added the Changelog: None Do not include in changelog label Jul 31, 2024
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* Use relative import for `_accelerate`

* Fix black

* Disable wrong-import-order in `__init__.py`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants