Running empty program puts QC object in bad state

[karalekas]

Pre-Report Checklist

• I am running the latest versions of pyQuil and the Forest SDK
• I checked to make sure that this bug has not already been reported

Issue Description

Running an empty Program results in an error and then puts the QuantumComputer in a bad state.

How to Reproduce

Code Snippet

from pyquil import get_qc, Program
qvm = get_qc('9q-qvm')
qvm.run(Program())

then

qvm.run(Program('X 0'))

Error Output

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-9-d1fd5c9f63f7> in <module>
----> 1 qvm.run(Program())

/src/pyquil/pyquil/api/_error_reporting.py in wrapper(*args, **kwargs)
    236             global_error_context.log[key] = pre_entry
    237
--> 238         val = func(*args, **kwargs)
    239
    240         # poke the return value of that call in

/src/pyquil/pyquil/api/_quantum_computer.py in run(self, executable, memory_map)
    125                     # TODO gh-658: have write_memory take a list rather than value + offset
    126                     self.qam.write_memory(region_name=region_name, offset=offset, value=value)
--> 127         return self.qam.run() \
    128             .wait() \
    129             .read_memory(region_name='ro')

/src/pyquil/pyquil/api/_error_reporting.py in wrapper(*args, **kwargs)
    236             global_error_context.log[key] = pre_entry
    237
--> 238         val = func(*args, **kwargs)
    239
    240         # poke the return value of that call in

/src/pyquil/pyquil/api/_qvm.py in run(self)
    517                                                         measurement_noise=self.measurement_noise,
    518                                                         gate_noise=self.gate_noise,
--> 519                                                         random_seed=self.random_seed)
    520
    521         if "ro" not in self._memory_results or len(self._memory_results["ro"]) == 0:

/src/pyquil/pyquil/api/_error_reporting.py in wrapper(*args, **kwargs)
    236             global_error_context.log[key] = pre_entry
    237
--> 238         val = func(*args, **kwargs)
    239
    240         # poke the return value of that call in

/src/pyquil/pyquil/api/_base_connection.py in _qvm_run(self, quil_program, classical_addresses, trials, measurement_noise, gate_noise, random_seed)
    359         """
    360         payload = qvm_run_payload(quil_program, classical_addresses, trials,
--> 361                                   measurement_noise, gate_noise, random_seed)
    362         response = post_json(self.session, self.sync_endpoint + "/qvm", payload)
    363

/src/pyquil/pyquil/api/_base_connection.py in qvm_run_payload(quil_program, classical_addresses, trials, measurement_noise, gate_noise, random_seed)
    240     """REST payload for :py:func:`ForestConnection._qvm_run`"""
    241     if not quil_program:
--> 242         raise ValueError("You have attempted to run an empty program."
    243                          " Please provide gates or measure instructions to your program.")
    244     if not isinstance(quil_program, Program):

ValueError: You have attempted to run an empty program. Please provide gates or measure instructions to your program.

then

/src/pyquil/pyquil/api/_qam.py in load(self, executable)
     52         if self.status == 'loaded':
     53             warnings.warn("Overwriting previously loaded executable.")
---> 54         assert self.status in ['connected', 'done', 'loaded']
     55
     56         self._variables_shim = {}

AssertionError:

Environment Context

Operating System: Debian Buster

Python Version (python -V): 3.6.9

Quilc Version (quilc --version): 1.12.0

QVM Version (qvm --version): 1.12.0

[ToJen]

Is this still open and what are some suggested modules to look at in order to fix this?

[appleby]

Yes, as far as I know this is still an issue. I'm actually not sure what correct fix here is though.

I'd recommend starting in the QuantumComputer.run method in pyquil/api/_quantum_computer.py, then follow along with the call graph into the corresponding QAM and QVM classes in pyquil/api/_qam.py and pyquil/api/_qvm.py, respectively.

Let us know if you have questions. Happy to review a PR or discuss in this thread.

[ToJen]

Cool cool, off the top of my head, sounds like some state is cached or not cleared. Will check it out.

[ToJen]

So here's what I found. As suspected, some state is not being reset. When qvm.run starts, it sets the status to running and does not reset this after the error. So I manually called qvm.reset() but this means you could lose all the computed in-memory data so far like connection, compiler etc. unless there's a way that those can be quickly reconfigured by injecting their dependencies...

What should probably be happening is that when qvm.run fails on an empty Program, the status should be reset immediately. This can be done by wrapping the assert in a try except?

    # https://sourcegraph.com/github.com/rigetti/pyquil/-/blob/pyquil/api/_qam.py#L58
    def load(self, executable: Union[BinaryExecutableResponse, PyQuilExecutableResponse]) -> "QAM":
        """
        Initialize a QAM into a fresh state.

        :param executable: Load a compiled executable onto the QAM.
        """
        self.status: str
        if self.status == "loaded":
            warnings.warn("Overwriting previously loaded executable.")
        assert self.status in ["connected", "done", "loaded"]
        # ^^^ this AssertionError should be handled ^^^

Or better still, status should only be set to running when load() is complete.
https://sourcegraph.com/github.com/rigetti/pyquil/-/blob/pyquil/api/_quantum_computer.py#L133

Here's my version of the snippet from the first comment on this issue:

from pyquil import get_qc, Program

qvm = get_qc('9q-qvm')

try:
    qvm.run(Program())
except:
    print("bad happened")

# set my debugger to watch the qvm variable and saw that qvm.qam.status remained 'running'
qvm.reset() # will revert status to 'connected'

qvm.run(Program('X 0'))

print(qvm)
# prints 9q-qvm

Those are my thoughts and there are probably better solutions. But it all centers around resetting the status of the machine.

[appleby]

What should probably be happening is that when qvm.run fails on an empty Program, the status should be reset immediately.

I'm not sure I like the idea of calling reset automatically in these cases. For example QAM.reset also resets the _memory_results dictionary, which would wipe out any accumulated state from previous runs. otoh, we apparently call reset on every compile when the underlying QAM is a QPU, so maybe it's not the end of the world.

Personally, I think a better approach would be to change all the asserts that are validating the state transitions inside the QAM class to instead raise an exception with a more helpful error message telling the user they can call qc.reset themselves if they want, but that they will lose any accumulated state. So for example the line

assert self.status in ["connected", "done", "loaded"]

would change to something like

if self.status not in ["connected", "done", "loaded"]:
   raise QAMStateTransitionError(current=self.status, expected= ["connected", "done", "loaded"])

or something like that.

Another option would be to add a check at the top of the compile method and raise an error there if the user tries to compile an empty program. That won't work for the example in the original bug description which is passing a bare Program to qc.run without first compiling, but technically that appears to be breaking the interface, as qc.run is declared to take an executable, not a Program.

There is a larger (philosophical) question of why attempting to run an empty program should be an error, but it seems we've already made that choice, so I'm happy to stick with it.

[appleby]

There is a larger (philosophical) question of why attempting to run an empty program should be an error, but it seems we've already made that choice, so I'm happy to stick with it.

I may have spoken too soon! It seems there is some debate about this very topic: #1182 (comment)

[ToJen]

Yes, I prefer this approach too.

would change to something like

if self.status not in ["connected", "done", "loaded"]:
   raise QAMStateTransitionError(current=self.status, expected= ["connected", "done", "loaded"])

Along the lines of what I was thinking when I wrote

This can be done by wrapping the assert in a try except?

What would the next steps be

• Creating the QAMStateTransitionError exception
• Changing the run() function in _quantum_computer.py
• Updating tests
• Making a PR

[appleby]

I think a two-pronged approach makes sense, namely:

1. Try to prevent entering a bad state when the user attempts to run an empty Program.
2. If we do wind up in an invalid state, print a more helpful error message letting the user know that they can call reset to put the QuantumComputer or QAM back into a useable state.

Regarding (1), I think adding a check near the top of QuantumComputer.compile and raising an exception if program is empty makes sense. This won't prevent the error in the original example, which never calls compile, but the docstring for QuantumComputer.run says that the caller is responsible for compiling the executable argument, so I think the compile method is a reasonable place to do the empty-program check.

Regarding (2), maybe a new exception class is overkill and we can just pass a helpful error message to the existing assert statements.

So next steps might be something like:

1. Add a check near the top of QuantumComputer.compile that raises ValueError if program is an empty Program.
2. Define a new "constant" in the pyquil/_api/_qam.py module, something like _QAM_BAD_STATE_ERR_MSG = "Invalid QAM state. The requested operation cannot be performed. You can call QAM.reset or QuantumComputer.reset to reset the state.
3. Replace all instances of assert self.status ... in pyquil/_api/_qam.py and pyquil/pyqvm.py with assert self.status ..., _QAM_BAD_STATE_ERR_MSG.
4. Add some tests

[CaineArdayfio]

Hello, has this issue been resolved? If not, I'd like to work on it.

[notmgsk]

Hey, @CaineArdayfio. This is still an open issue. Take a look at #1034 (comment) for some suggestions on how to tackle this. :D