search

Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
4.85k stars 2.29k forks source link

qiskit_qasm3 does not build with PyPy #11822

Closed wshanks closed 4 months ago

wshanks commented 4 months ago

Environment

What is happening?

Qiskit fails to build when using PyPy. Specifically, the qiskit_qasm3 crate fails with

  error[E0599]: no method named `filename` found for reference `&pyo3::prelude::PyModule` in the current scope
    --> crates/qasm3/src/lib.rs:68:34
     |
  68 |         Ok(vec![Path::new(module.filename()?)
     |                                  ^^^^^^^^ help: there is a method with a similar name: `name`

  For more information about this error, try `rustc --explain E0599`.
  error: could not compile `qiskit-qasm3` (lib) due to 1 previous error

How can we reproduce the issue?

Attempt to run pip install . with PyPy3.9.

What should happen?

I am not sure :slightly_smiling_face: I know of this page specifying the different tiers of support for operating system versions, but I could not find anything stating if PyPy should be supported. I am just reporting that Qiskit 0.46 does build with PyPy and Qiskit 1.0 does not.

Any suggestions?

The issue is that PyModule does not have the filename function when using PyPy. The function was disabled here though there is not much explanation for why. Here is where the function is defined in PyO3.

Also, I don't personally use Qiskit with PyPy. I was just porting the conda build from 0.46 to 1.0 and found this.

jakelishman commented 4 months ago

Woah, haha. Do you know what version of PyO3 is being used in the lockfile for that build?

jakelishman commented 4 months ago

I would not imagine that any component of this would actually work with PyPy, but who knows - I don't really know the details of how their hook points to the CPython C API work. Certainly we've never attempted to build PyPy wheels on our side.

jakelishman commented 4 months ago

Ohhhh wait sorry, I just clicked through. Yeah, I guess if modules don't have filename attributes on PyPy, then that just isn't going to work. Do you know if they have __file__ attributes in Python space / is it just the C-API side that's missing?

wshanks commented 4 months ago

Do you know what version of PyO3 is being used in the lockfile for that build?

I force-pushed so the failed build is hard to find. It is here. The pyo3 version was v0.20.2.

Certainly we've never attempted to build PyPy wheels on our side.

Yes, I haven't used them either, so I don't know how high a priority this is, but it was at least building in the terra feedstock here.

Do you know if they have file attributes in Python space / is it just the C-API side that's missing?

$ pypy -c "import os; print(os.__file__)"
/home/wshanks/.conda/envs/pypy/lib/pypy3.9/os.py

It seems to work for Python files. It's kind of strange. There's not much information in that PyO3 commit that disabled the function for PyPy.

jakelishman commented 4 months ago

Flicking through, it looks like PyO3 uses the semi-private _PyModule_GetFilename function to power that method, which PyPy presumably doesn't include. In theory, that line is super minor and nowhere even close to performance critical, so I could switch it to a Python-space access to __file__, which I imagine would work in PyPy. As we use Rust more and more, though, it's ever more likely that we'll break the PyPy build by using some other C API stuff that's not compatible with PyPy, and we won't catch that in CI, because we don't really have the resources to test PyPy.

So, while the particular fix here would be super easy, I guess the question is: do we want to keep playing whack-a-mole with PyPy issues post release, when the conda-feedstock tries the build and tells us about them, or do we just want to entirely write off PyPy support now? We've never really considered it before, and I didn't actually realise we were ever trying to build PyPy wheels.

We can also punt the question by fixing this particular issue and seeing if further issues actually turn up.

mtreinish commented 4 months ago

Yeah this is the first time someone has reported this. FWIW, I saw this coming years ago when we first started using rustworkx as a dependency (and that we don't build pypy wheels for rustworkx) and it's why I put:

Additionally, Qiskit only supports CPython. Running with other Python interpreters is not supported.

in the platform support documentation:

https://docs.quantum.ibm.com/start/install#operating-system-support

That being said how important is this for the conda build system? I would expect a lot of our dependencies don't support pypy either.

EDIT: Oops missed you called out CPython is the only supported platform in the docs already. I will say if we can make it work for you on pypy with very little effort we should do that. It's more this will likely happen again in the future as we use more Rust code in Qiskit and we don't test or build for pypy anywhere.

wshanks commented 4 months ago

it's why I put:

Additionally, Qiskit only supports CPython. Running with other Python interpreters is not supported.

in the platform support documentation:

Ack, I linked right to that part of the documentation above but glossed over that statement since the section was name "operating system support". Perhaps one action item would be to update that section to be "Operating system and Python implementation support" and then at "other Python interpreters" add "such as PyPy" so it comes up more readily in a search for "qiskit pypy". I am not sure if there are any other likely cases besides PyPy that someone might search for? Maybe wasm/wasi which have been getting more attention recently.

That being said how important is this for the conda build system? I would expect a lot of our dependencies don't support pypy either.

I don't think it has any particular importance. The other maintainer of qiskit-terra had enabled PyPy builds, probably because the automation opens a PR to enable them by default and the build ran without errors. I could just drop PyPy builds and see if anyone asks for them. I opened the issue because they had been building and stopped with 1.0 and I wasn't sure if it was "don't support at all" or a "best effort to keep working" kind of issue.

The automation also tried to enable PyPy for Aer but ran into a build error. I didn't understand the build error (but might have been due to missing support from pybind11), so I left the PyPy PR unmerged with a comment that it could be looked at more if anyone wanted it. That was a year ago and I haven't heard anything so far.

With conda install qiskit pypy stestr ddt (so no optionals, mainly because I forgot about them before starting the tests), this was the result for stestr run:

======
Totals
======
Ran: 21975 tests in 2358.0034 sec.
 - Passed: 21476
 - Skipped: 484
 - Expected Fail: 5
 - Unexpected Success: 0
 - Failed: 10
Sum of execute time for each test: 16864.2666 sec.

Here is the full failure set. Some of the failures might reflect some hidden assumptions that could be fixed for CPython. It looks like TestOperator.test_drawings is missing a HAS_IPYTHON and maybe some of the tests assume ordering where there isn't guaranteed ordering.

``` ====================================================================== FAIL: test.python.dagcircuit.test_dagdependency.TestDagNodeSelection.test_successors_predecessors tags: worker-4 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/dagcircuit/test_dagdependency.py", line 229, in test_successors_predecessors self.assertEqual(successors_second, [2, 4, 5]) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 1043, in assertListEqual self.assertSequenceEqual(list1, list2, msg, seq_type=list) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 1025, in assertSequenceEqual self.fail(msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 676, in fail raise self.failureException(msg) AssertionError: Lists differ: [4, 2, 5] != [2, 4, 5] First differing element 0: 4 2 - [4, 2, 5] + [2, 4, 5] ====================================================================== FAIL: test.python.quantum_info.operators.test_dihedral.TestCNOTDihedral.test_dot_method tags: worker-0 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/quantum_info/operators/test_dihedral.py", line 786, in test_dot_method value = elem1.dot(elem2) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/mixins/group.py", line 149, in dot return self.compose(other, qargs=qargs, front=True) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/dihedral.py", line 380, in compose other = self._dot(other) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/dihedral.py", line 217, in _dot result.poly = other.poly + self.poly.evaluate(new_vars) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 159, in evaluate newterm = reduce(mul, [xval[j] for j in term], start) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 98, in __mul__ temp = temp.mul_monomial(term) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 74, in mul_monomial result.set_term(new_term, (result.get_term(new_term) + value) % 8) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 206, in get_term raise QiskitError("Indices are non-increasing.") qiskit.exceptions.QiskitError: 'Indices are non-increasing.' ====================================================================== FAIL: test.python.quantum_info.operators.test_operator.TestOperator.test_drawings tags: worker-0 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/visualization/array.py", line 197, in array_to_latex from IPython.display import Latex ModuleNotFoundError: No module named 'IPython' The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/quantum_info/operators/test_operator.py", line 694, in test_drawings op.draw("latex") File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/operator.py", line 200, in draw return array_to_latex(self, **drawer_args) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/visualization/array.py", line 199, in array_to_latex raise MissingOptionalLibraryError( qiskit.exceptions.MissingOptionalLibraryError: "The 'IPython' library is required to use 'array_to_latex'. You can install it with 'pip install ipython'." ====================================================================== FAIL: test.python.transpiler.test_layout_transformation.TestLayoutTransformation.test_three_qubit tags: worker-4 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/transpiler/test_layout_transformation.py", line 47, in test_three_qubit self.assertEqual(circuit_to_dag(expected), output_dag) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 830, in _baseAssertEqual raise self.failureException(msg) AssertionError: != ====================================================================== FAIL: test.python.result.test_result.TestResultOperations.test_result_repr tags: worker-7 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/result/test_result.py", line 127, in test_result_repr self.assertEqual(expected, repr(result)) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 1217, in assertMultiLineEqual self.fail(self._formatMessage(msg, standardMsg)) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 676, in fail raise self.failureException(msg) AssertionError: "Resu[230 chars]ader=QobjExperimentHeader(creg_sizes=[['c0', 2[77 chars]one)" != "Resu[230 chars]ader=namespace(creg_sizes=[['c0', 2], ['c0', 1[66 chars]one)" Diff is 1231 characters long. Set self.maxDiff to None to see it. ====================================================================== FAIL: test.python.quantum_info.operators.test_dihedral.TestCNOTDihedral.test_compose_method tags: worker-6 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/quantum_info/operators/test_dihedral.py", line 772, in test_compose_method value = elem1.compose(elem2) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/dihedral.py", line 382, in compose other = self._compose(other) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/dihedral.py", line 240, in _compose result.poly = self.poly + other.poly.evaluate(new_vars) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 159, in evaluate newterm = reduce(mul, [xval[j] for j in term], start) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 98, in __mul__ temp = temp.mul_monomial(term) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 74, in mul_monomial result.set_term(new_term, (result.get_term(new_term) + value) % 8) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/site-packages/qiskit/quantum_info/operators/dihedral/polynomial.py", line 206, in get_term raise QiskitError("Indices are non-increasing.") qiskit.exceptions.QiskitError: 'Indices are non-increasing.' ====================================================================== FAIL: test.python.transpiler.test_layout_transformation.TestLayoutTransformation.test_four_qubit tags: worker-1 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/transpiler/test_layout_transformation.py", line 67, in test_four_qubit self.assertEqual(circuit_to_dag(expected), output_dag) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 830, in _baseAssertEqual raise self.failureException(msg) AssertionError: != ====================================================================== FAIL: test.python.transpiler.test_sabre_layout.TestSabrePreLayout.test_starting_layout tags: worker-7 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/transpiler/test_sabre_layout.py", line 418, in test_starting_layout self.assertEqual( File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 1043, in assertListEqual self.assertSequenceEqual(list1, list2, msg, seq_type=list) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 1025, in assertSequenceEqual self.fail(msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 676, in fail raise self.failureException(msg) AssertionError: Lists differ: [69, 109, 43, 110, 44, 111, 70, 105, 37, 104, 98, 29, 97, 28, 65, 103] != [30, 98, 104, 36, 103, 35, 65, 28, 61, 91, 22, 92, 23, 93, 62, 99] First differing element 0: 69 30 - [69, 109, 43, 110, 44, 111, 70, 105, 37, 104, 98, 29, 97, 28, 65, 103] + [30, 98, 104, 36, 103, 35, 65, 28, 61, 91, 22, 92, 23, 93, 62, 99] ====================================================================== FAIL: test.python.quantum_info.states.test_utils.TestStateUtils.test_schmidt_decomposition_separable_with_permutation tags: worker-5 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/quantum_info/states/test_utils.py", line 82, in test_schmidt_decomposition_separable_with_permutation self.assertEqual(schmidt_comps[0][2], Statevector.from_label("l1")) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 830, in _baseAssertEqual raise self.failureException(msg) AssertionError: Statevector([-0. +0.j , -0. +0.j [92 chars], 2)) != Statevector([0. +0.j , 0.70710678+0.j [88 chars], 2)) ====================================================================== FAIL: test.python.transpiler.test_layout_transformation.TestLayoutTransformation.test_four_qubit_with_target tags: worker-2 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/wshanks/Documents/Code/qiskit-terra/test/python/transpiler/test_layout_transformation.py", line 86, in test_four_qubit_with_target self.assertEqual(circuit_to_dag(expected), output_dag) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 837, in assertEqual assertion_func(first, second, msg=msg) File "/home/wshanks/.conda/envs/pypy/lib/pypy3.9/unittest/case.py", line 830, in _baseAssertEqual raise self.failureException(msg) AssertionError: != Ran 10 tests in 1323.018s FAILED (id=20, failures=10) ```
wshanks commented 4 months ago

Regarding playing whack-a-mole, I asked the PyPy maintainers about C-API coverage. PyPy should have pretty good coverage of the limited stable C-API, which I think Qiskit is opting into by using the abi3 option of PyO3. One of the PyPy maintainers told me that API methods that return char * are hard for PyPy to deal with. In this case, PyModule_GetFilename is actually deprecated in favor of PyModule_GetFilenameObject. That was also not implemented in PyPy but the maintainer added an implementation for it when answering my question. Maybe Qiskit should switch to using PyModule_GetFilenameObject any way?

jakelishman commented 4 months ago

We don't really make a choice between PyModule_GetFilename and PyModule_GetFilenameObject - we use just the high-level PyO3 bindings in their PyModule struct, which PyO3 previously had as using PyModule_GetFilename in the low-level FFI layer.

Looks like PyO3 switched over to PyModule_GetFilenameObject in PyO3/pyo3#1425 which is destined for the omnibus 0.21 release, so if PyPy will add support for that in a later release, we can ask the PyO3 guys about relaxing the #[cfg(not(PyPy))] on the method at an appropriate time.

In the mean time, I suppose the question for us is whether we want to accept #[cfg(PyPy)] alternative paths within Qiskit itself, when we explicitly don't support PyPy in our wheel builds[^1]. I'm not super keen to degrade the tier-1 CPython version by using an uglier Python-space access to the __file__ attribute when PyO3 has a high-level method to access PyModule::filename, but I'm also not super keen on manual #[cfg(PyPy)] stuff for an interpreter we officially don't support.

[^1]: btw, when I said earlier about "we haven't discussed PyPy support", I meant along the lines of what we'd do in this sort of situation, i.e., would we make a best-effort-only attempt, or just abandon it at the first sign of trouble.

wshanks commented 4 months ago

We don't really make a choice between PyModule_GetFilename and PyModule_GetFilenameObject - we use just the high-level PyO3 bindings in their PyModule struct, which PyO3 previously had as using PyModule_GetFilename in the low-level FFI layer.

Oh, right, I forgot that you had mentioned PyModule_GetFilename and the original error message was just flagging filename(). To me, it looks like PyO3 has been using GetFilenameObject for a while but I might not be looking in the right place. In any case, it seems like PyO3 and PyPy will be aligned on using PyModule_GetFilenameObject in the future and Qiskit should not need to make any change to its code for this.

In the mean time, I suppose the question for us is whether we want to accept #[cfg(PyPy)] alternative paths within Qiskit itself

I think we can just maintain the status quo for now of not considering PyPy until someone asks for it but consider making small changes for PyPy if they come up and are otherwise code quality improvements (like if Qiskit actually had been using a deprecated method when there was an alternative that also worked with PyPy, or looking into those test failures). It seems like PyO3 tries to keep good PyPy support and with Qiskit using abi3 that might be enough to keep it working.

Besides the test failures, I think the only action item is the documentation change I suggested which I implemented in https://github.com/Qiskit/documentation/pull/844.

In the documentation, three tiers of support are defined where support correlates to testing and and publishing binaries. It is stated that Qiskit could be built for other platforms. The discussion doesn't touch on what level of source code modification would be tolerated for running on other platforms, just which ones get tested and get binaries published. The text also distinguishes other interpreters as not supported but I am not sure if that is a stronger statement than just not being listed in the three tiers.

jakelishman commented 4 months ago

That all sounds like a plan to me, and thanks for updating the docs, Will.

Shall we close this issue as a "won't fix" for the time being, and re-evaluate in the future if we turn up a more pressing demand?

wshanks commented 4 months ago

Sure, we can close this now. I will comment here if PyPy and PyO3 updates allow the Qiskit build to start working again.