amazon-braket / amazon-braket-default-simulator-python

Quantum program simulators that can run locally
https://amazon-braket-default-simulator-python.readthedocs.io/
Apache License 2.0
67 stars 22 forks source link

AttributeError: 'ArrayLiteral' object has no attribute 'value'. #240

Closed DanBlackwell closed 5 months ago

DanBlackwell commented 7 months ago

Describe the bug In general the openQASM parsing errors are well handled in Braket. There are some cases where it is assumed that an object will have a .value and the exception is not caught. These are from auto-generated programs, so might not be an issue in practice; but perhaps the error message could be improved to include context if the exception was caught.

I'm not sure that this is really a bug, so please let me know if not and I will delete the issue. The example I include here is for ArrayLiterals, but there are a whole family of related errors I have triggered.

To reproduce

from braket.circuits import Circuit
from braket.devices import LocalSimulator

device = LocalSimulator()
Circuit.from_ir('''
OPENQASM 3.0;
include "stdgates.inc";
qubit["10"] r1;
''')

Expected behavior Either return an error message indicating that bitstrings are not allowed for declaring register sizes, or parse the bitstring into a numeric constant (2) and create an array with that size.

Screenshots or logs

Traceback (most recent call last):
  File "/harnesses/./differential_harness.py", line 291, in <module>
    assert run_and_compare_exact_probabilities(qasm_content, 0.01)
  File "/harnesses/./differential_harness.py", line 177, in run_and_compare_exact_probabilities
    braket_circuit = braket_sim.build_circuit(qasm_content)
  File "/harnesses/./differential_harness.py", line 32, in build_circuit
    quantum_circuit = BraketCircuit.from_ir(qasm_content)
  File "/usr/local/lib/python3.10/dist-packages/braket/circuits/circuit.py", line 1167, in from_ir
    return Interpreter(BraketProgramContext()).build_circuit(
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 130, in build_circuit
    return self.run(source, inputs, is_file).circuit
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 145, in run
    self.visit(program)
  File "/usr/lib/python3.10/functools.py", line 926, in _method
    return method.__get__(obj, cls)(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 172, in _
    self.visit(node.statements)
  File "/usr/lib/python3.10/functools.py", line 926, in _method
    return method.__get__(obj, cls)(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 168, in _
    return [n for n in [self.visit(node) for node in node_list] if n is not None]
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 168, in <listcomp>
    return [n for n in [self.visit(node) for node in node_list] if n is not None]
  File "/usr/lib/python3.10/functools.py", line 926, in _method
    return method.__get__(obj, cls)(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 250, in _
    size = self.visit(node.size).value if node.size else 1

System information A description of your system. Please provide:

Additional context N/A

ajberdy commented 7 months ago

Hi, thanks for raising! As per the OpenQASM spec here, the qubit register size must be a compile-time constant (an integer literal or const variable of an integer type). In this case, it will always have the value attribute.

I notice in your example the qubit register size is specified with a string literal, which is not defined syntax. An action item here would be to improve our error handling here to make the problem more clear to users.

DanBlackwell commented 7 months ago

I agree that this is an unusual (perhaps unrealistic) use case, but interestingly the grammar defines a BitstringLiteral (https://openqasm.com/grammar/index.html#:~:text=%3B%0A156%0A157-,BitstringLiteral,-%3A%20%27%22%27%20), and Qiskit itself handles definitions with the bitstring being expanded as 0b10 would in Python or other languages. Whether or not that is really what the spec says is a different matter.

Anyway, just thought I'd mention it as the error message was not as helpful as they were in other places.

ajberdy commented 7 months ago

Good point. This bug can be fixed by casting the node size into an integer with before accessing value. Something like

@visit.register
    def _(self, node: QubitDeclaration) -> None:
        size_arg = self.visit(node.size)
        size = cast_to(IntType, size_arg).value if size_arg else 1
        self.context.add_qubits(node.qubit.name, size)
DanBlackwell commented 7 months ago

I have a bunch of other ones that are invalid according to the spec, but produce similar error messages that could be improved by handling the exception and outputting more context such as the line number or token.

For example:

from braket.circuits import Circuit
from braket.devices import LocalSimulator

device = LocalSimulator()
quantum_circuit = Circuit.from_ir('''
OPENQASM 3.0;
include "stdgates.inc";
qubit[1] r1;
h r1["0" << "1"];
''')

produces this:

  ...
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/interpreter.py", line 216, in _
    return evaluate_binary_expression(lhs, rhs, node.op)
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/functions.py", line 171, in evaluate_binary_expression
    return func(lhs, rhs)
  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/functions.py", line 101, in <lambda>
    x.values[y.value :] + [BooleanLiteral(False) for _ in range(y.value)]
AttributeError: 'ArrayLiteral' object has no attribute 'value'. Did you mean: 'values'?

Are these useful to you? Perhaps I could drop them into another issue and mark it as an enhancement.

ajberdy commented 7 months ago

It looks like that example is the same failure mode of a BitString not having a value field. The casting fix should cover all of these cases assuming the others are similar.

DanBlackwell commented 7 months ago

True, once you have a PR I can retest them and let you know if that gets them all.

manulpatel commented 5 months ago

Hi @ajberdy! I would like to fix this issue. Could you please assign me?

rmshaffer commented 5 months ago

@DanBlackwell There is now a proposed PR #258 to fix this issue. If you have any feedback on that, it would be very welcome.

DanBlackwell commented 5 months ago

@rmshaffer thanks for informing me, I've dropped a comment on the PR. It looks like it fixes the errors that were mentioned in this issue. I believe I have some others that it might not fix, so I'll drop minimal QASM examples down below (note that I don't think these are valid programs according to the QASM spec, just that the error message could be improved):

1

qubit[1] r1;
for int lv1 in [true << "01101":bit(0.766417482119969) >> !true:-!r2] a r1;

gives:

  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/functions.py", line 101, in <lambda>
    x.values[y.value :] + [BooleanLiteral(False) for _ in range(y.value)]
AttributeError: 'BooleanLiteral' object has no attribute 'values'. Did you mean: 'value'?

2

creg r1[$17];
qreg r2[1];
x r1[0];

gives (I believe due to the $17):

  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/arrays.py", line 109, in create_empty_array
    return ArrayLiteral([None] * dims[0].value)
AttributeError: 'Identifier' object has no attribute 'value'

3

qubit[(!~bit(!$1))] r1;

gives:

  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/casting.py", line 54, in cast_to
    return BooleanLiteral(bool(variable.value))
AttributeError: 'UnaryExpression' object has no attribute 'value'

4

bit r2 = measure $12;

gives:

  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/casting.py", line 54, in cast_to
    return BooleanLiteral(bool(variable.value))
AttributeError: 'NoneType' object has no attribute 'value'

5

bit[bit(bit(!"001"))] r2 = {{{{!bit["01101"](2.8970973) > g1}, -pi}}, false};

gives:

  File "/usr/local/lib/python3.10/dist-packages/braket/default_simulator/openqasm/_helpers/casting.py", line 76, in _
    not all(isinstance(x, BooleanLiteral) for x in variable.values)
AttributeError: 'FloatLiteral' object has no attribute 'values'. Did you mean: 'value'?
DanBlackwell commented 5 months ago

I believe that #258 does fix this particular issue though; the above AttributeErrors are far from valid QASM.

rmshaffer commented 5 months ago

Thanks @DanBlackwell! This is very helpful feedback and I agree with your assessment.

DanBlackwell commented 5 months ago

fwiw, I did make a start at producing error messages with line number and position (instead of just AttributeError); but I got confused about what exactly Span is here. I was hoping that it would line up with the QASM source, but it didn't; maybe it's the position after tokenisation / lexing?

DanBlackwell commented 5 months ago

Also, I did try tagging a question onto the end of another issue with the Qiskit team here to clarify this https://github.com/Qiskit/qiskit/issues/12167#issuecomment-2110468637; but I guess no one saw it. From that thread it seemed like levbishop was the most knowledgable about casting.

rmshaffer commented 5 months ago

@EuGig, thank you for your contribution in #258! I have merged this PR into a feature branch, and we will be close this issue for now (to ensure that you get credit for this in unitaryHACK). I have opened #263 to track getting the changes merged to main, and we will keep this issue updated with the status.

@EuGig, would you please reply to this comment confirming that you would like this issue to be assigned to you? (GitHub requires that you comment on an issue before it can be assigned to you.)

EuGig commented 5 months ago

@rmshaffer I confirm I would like this issue to be assigned to me!