Add quilc_client property to QVMCompiler, improve timeout documentation #1273

notmgsk · 2020-11-16T20:38:28Z

Add a client property to QPUCompiler so that it has better feature
parity with QVMCompiler. Generally we tell folks that

QVMCompiler and QPUCompiler should be largely interchangeable,
meaning that you can swap qc = get_qc("Aspen-8", as_qvm=True) with
qc = get_qc("Aspen-8", as_qvm=False), and things should Just
Work. And
To set the timeout on a QVMCompiler one does
qc.compiler.client.rpc_timeout = ... but on a QPUCompiler
qc.compiler.quilc_client.rpc_timeout = ....

This change brings 1 and 2 into alignment, so that the method for
configuring the compiler timeout is the same between the compiler
classes.

Further, it adds a set_timeout method for both classes, and updates
the documentation.

Add a client property to QPUCompiler so that it has better feature parity with QVMCompiler. Generally we tell folks that 1. QVMCompiler and QPUCompiler should be largely interchangeable, meaning that you can swap `qc = get_qc("Aspen-8", as_qvm=True)` with `qc = get_qc("Aspen-8", as_qvm=False)`, and things should Just Work. And 2. To set the timeout on a QVMCompiler one does `qc.compiler.client.rpc_timeout = ...` but on a QPUCompiler `qc.compiler.quilc_client.rpc_timeout = ...`. This change brings 1 and 2 into alignment, so that the method for configuring the compiler timeout is the same between the compiler classes.

mhodson-rigetti · 2020-11-16T21:34:10Z

docs/source/compiler.rst

+
+.. code:: python
+    qc = get_qc(..., as_qvm=False)
+    qc.compiler.quilc_client.rpc_timeout = 100 # 100 seconds


Is it? The instructions I've always used / given is:

On QVM, you need "qc.compiler.client.timeout";
On QPU it is just "qc.compiler.timeout".

The timeout is most relevant to the Client object that is created. In both QVMCompiler and QPUCompiler the (quilc) client is instantiated in __init__ and so immediately uses the timeout arg given to __init__ (this is the same value that is assigned to self.timeout in both classes). Therefore if you want to modify the timeout that is used specifically in quilc, then you have to change qc.compiler.{client,quilc_client}.rpc_timeout. QPUCompiler has a second client to the translation service, which also uses self.timeout but is deferred until the client is requested. I think that means that you probably got it to work by using qc.compiler.timeout because the client hadn't yet been created, and so used the new timeout value.

All that to say, this PR doesn't modify the translation service timeout. Should it?

Yeah, unless there was a separate well-documented path for the translation service, I'd apply this one timeout in both locations (and say that it applies to both the compiler timeout for quil_to_native_quil, and translation timeout for native_quil_to_executable).

Note that a commit I just added now does modify the translation service timeout. That one is set on a per-call basis to the current value of QPUCompiler#timeout.

pyquil/api/_compiler.py

docs/source/compiler.rst

kalzoo · 2020-11-16T23:08:46Z

pyquil/api/_compiler.py

+    @property
+    def client(self) -> Client:
+        """Return the `Client` for the compiler (i.e. quilc, not translation service)."""
+        # TODO(notmgsk): This was introduced around 2.25 to provide


This should be in the docstring, and not version-specific. While that seems like important information within the context of the PR, it becomes irrelevant as more information comes out.

Also, this might be worth having as an @abstractmethod on the AbstractCompiler with the documentation note there instead. That's the intent behind abstract methods in any case - parity among subclasses.

I removed the comment entirely, and have chosen to not take the abstract method route just because at this point I'm not sure how much it gives us, but if you feel strongly about it I will add it.

well, now that the methods are identical, I think it's wise to make it a non-abstract method on the parent class for de-duplication

kalzoo · 2020-11-16T23:09:51Z

pyquil/api/_compiler.py

@@ -182,7 +182,7 @@ def __init__(
        quilc_endpoint: Optional[str],
        qpu_compiler_endpoint: Optional[str],
        device: AbstractDevice,
-        timeout: int = 10,
+        timeout: float = 10,


Should we also support this in get_qc, maybe as compiler_timeout? That's the "tutorial" way of constructing this in any case.

Donezo. I have refrained from adding a timeout option to QuantumComputer#compile, because (1) I think the options available now are sufficient, and (2) we could probably find a million places to add a timeout flag to (gotta draw the line somewhere).

pyquil/api/_compiler.py

docs/source/compiler.rst

kalzoo · 2020-11-17T04:51:47Z

pyquil/api/_compiler.py

@@ -338,7 +338,9 @@ def native_quil_to_executable(self, nq_program: Program) -> Optional[BinaryExecu
        )
        response: BinaryExecutableResponse = cast(
            BinaryExecutableResponse,
-            self.qpu_compiler_client.call("native_quil_to_binary", request),
+            self.qpu_compiler_client.call(
+                "native_quil_to_binary", request, rpc_timeout=self.timeout


I flubbed this in #1246 , should have been included there

kalzoo · 2020-11-17T04:53:04Z

docs/source/compiler.rst

+
+.. code:: python
+    qc = get_qc(..., as_qvm=False)
+    qc.compiler.quilc_client.rpc_timeout = 100 # 100 seconds


Note that a commit I just added now does modify the translation service timeout. That one is set on a per-call basis to the current value of QPUCompiler#timeout.

kalzoo · 2020-11-17T17:03:51Z

Needs a changelog entry also.

notmgsk · 2020-11-17T20:06:59Z

Hopefully all comments are addressed. Please take another look

kalzoo

~~Since set_timeout is now identical between the two compilers, it should be hoisted to a concrete method on the parent class. Otherwise,~~ lgtm

notmgsk added 3 commits November 16, 2020 20:31

fix type

7a98277

Add set_timeout method to {QVM,QPU}Compiler classes

95a8bc3

notmgsk requested review from kalzoo, erichulburd and mhodson-rigetti November 16, 2020 21:06

notmgsk marked this pull request as ready for review November 16, 2020 21:06

notmgsk requested a review from a team as a code owner November 16, 2020 21:06

mhodson-rigetti reviewed Nov 16, 2020

View reviewed changes

fix doc

45b176a

kalzoo requested changes Nov 16, 2020

View reviewed changes

erichulburd requested changes Nov 17, 2020

View reviewed changes

pyquil/api/_compiler.py Show resolved Hide resolved

docs/source/compiler.rst Outdated Show resolved Hide resolved

kalzoo added 2 commits November 16, 2020 21:49

Fix: send timeout value in native_quil_to_binary call

e336558

Fix: set_timeout docstring

400e21d

kalzoo reviewed Nov 17, 2020

View reviewed changes

notmgsk added 5 commits November 17, 2020 19:36

Remove QPUCompiler#client in favour of QVMCompiler#quilc_client

c9f0464

Remove some docs

dc69e4b

Remove comment

18dceef

Add compiler_timeout option to get_qc()

abe8e90

Note timeout units

bceb3ca

notmgsk requested review from kalzoo and erichulburd November 17, 2020 20:07

notmgsk added 2 commits November 17, 2020 20:08

oops

fc4b5e4

type int -> float

a35d209

notmgsk changed the title ~~Add client property to QPUCompiler, improve timeout documentation~~ Add quilc_client property to QVMCompiler, improve timeout documentation Nov 17, 2020

notmgsk added 2 commits November 17, 2020 21:16

update changelog

cc8cf9a

remove duplicate line

152defe

kalzoo requested changes Nov 17, 2020

View reviewed changes

kalzoo approved these changes Nov 17, 2020

View reviewed changes

notmgsk merged commit 30f33b8 into master Nov 17, 2020

notmgsk mentioned this pull request Nov 17, 2020

Bump version to 2.25.0 #1274

Merged

dbanty deleted the document-timeout-configuration branch February 14, 2022 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quilc_client property to QVMCompiler, improve timeout documentation #1273

Add quilc_client property to QVMCompiler, improve timeout documentation #1273

notmgsk commented Nov 16, 2020 •

edited

Loading

mhodson-rigetti Nov 16, 2020

notmgsk Nov 16, 2020 •

edited

Loading

mhodson-rigetti Nov 16, 2020

kalzoo Nov 17, 2020

kalzoo Nov 16, 2020

notmgsk Nov 17, 2020

kalzoo Nov 17, 2020

kalzoo Nov 16, 2020

notmgsk Nov 17, 2020

kalzoo Nov 17, 2020

kalzoo Nov 17, 2020

kalzoo Nov 17, 2020

kalzoo commented Nov 17, 2020

notmgsk commented Nov 17, 2020

kalzoo left a comment •

edited

Loading

Add quilc_client property to QVMCompiler, improve timeout documentation #1273

Add quilc_client property to QVMCompiler, improve timeout documentation #1273

Conversation

notmgsk commented Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

notmgsk Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalzoo commented Nov 17, 2020

notmgsk commented Nov 17, 2020

kalzoo left a comment • edited Loading

Choose a reason for hiding this comment

notmgsk commented Nov 16, 2020 •

edited

Loading

notmgsk Nov 16, 2020 •

edited

Loading

kalzoo left a comment •

edited

Loading