Add CQCHandler abstract class, make CQCConnection subclass of it, add CQCToFile class for writing commands to file #39

lgvacanti · 2019-10-16T09:50:41Z

Main changes:

New abstract class CQCHandler. This has some common functionality that is needed in both CQCConnection and CQCToFile. It also defines some abstractmethods that they must replace.
CQCConnection has been made a subclass of CQCHandler
New class CQCToFile. It is a subclass of CQCHandler. It allows writing the CQC commands to a file.

AckslD · 2019-10-16T13:16:14Z

cqc/pythonLib.py

+        self.pending_messages = []
+
+        # Set an app ID
+        self._appID = 6


Why six? I think the functionality for increasing the appID could be moved to the abstract class

It was just a number that made it easier to recognize in the printed out commands. There is no real reason for it.

AckslD · 2019-10-16T13:17:41Z

Hi @shyadow! Nice work :) I think it would be better as we talked about to instead have subclasses implement the sending and reaceiving of messages. In this way most things can go the the abstract class, such as sendCommand, sendXtra etc. What do you think?

lgvacanti · 2019-10-16T13:26:31Z

Hi @shyadow! Nice work :) I think it would be better as we talked about to instead have subclasses implement the sending and reaceiving of messages. In this way most things can go the the abstract class, such as sendCommand, sendXtra etc. What do you think?

@AckslD. So you mean sendCommand would for example do:

self.construct_command()
self.send()

and send() would be the method that the subclasses have to replace. It would be sending over socket for CQCConnection and writing to file for CQCToFile?

I like the idea. I only wonder if there is a better name than send?

AckslD · 2019-10-16T13:33:09Z

@shyadow Yes, exactly. There might be a better name, forward? execute? transmit? push? Feel free to choose a name you think is suitable.

lgvacanti · 2019-10-16T13:34:44Z

@AckslD Alright, I'll work on implementing that.

lgvacanti · 2019-10-16T14:02:31Z

@AckslD sendCommand. sendCmdXtra, and sendSimple have been moved to CQCHandler and commit() is now an abstract method that needs to be replaced by the subclasses.

I feel like you can generally say "I'm committing this message" and it can then mean committing to send it or committing to write it depending on what you are doing.

lgvacanti · 2019-10-17T09:10:10Z

@AckslD The construct commands have been merged. Also noticed sendCommand and sendCmdXtra could then be merged too, so those have been replaced by commit_command().

AckslD · 2019-10-17T12:47:36Z

@shyadow Let me know when you want me to take another look

lgvacanti · 2019-10-17T12:49:31Z

@AckslD It should be good now.

- Add CQCHandler abstract class - Make CQCConnection a subclass of CQCHandler - Add CQCWriteToFile class, which is a subclass of CQCHandler - Add tests for CQCWriteToFile

…qubits

Note: In Develop, when creating a new qubit it send CQC Header and Command Header seperately. Here it sends them together. So a test needed to be changed to reflect that.

lgvacanti · 2019-11-13T14:01:27Z

@AckslD I brought this pull request up to date with the Develop branch

AckslD · 2019-11-15T16:01:51Z

@lgvacanti There are now tests and the docs are updated with the changes Victor did. So you can know rebase on Develop again and we can now prepare to merge this. Btw, it would be great if you can update the docs as well on how to use the CQCToFile.

lgvacanti · 2019-11-20T11:27:51Z

@AckslD It should be good to start reviewing now.

AckslD

Nice work @lgvacanti! I had a few comments, see below

AckslD · 2019-11-26T15:29:56Z

docs/CQCToFile.rst

+another file with the same name, but with 'binary' appended, which will contain
+the commands in binary form (which is easier to process later).
+
+:meth:`cqc.pythonLib.CQCToFile` does not support sending or receiving 


Are there other commands not supported, like get-creation-time etc?

I added sendGetTime(), allocate_qubits(), sendFactory(), create_qubits(),
tomography(), test_preparation() to the list.

There are some more methods in CQCConnection which are not available in CQCToFile, but they are things that usually only get used internally, I think, such as startClassicalServer() and some others.

AckslD · 2019-11-26T15:32:26Z

tests/test_CQCToFile.py

+        with open(filename) as f:
+            contents = f.read()
+            print(contents)
+            assert contents[6:10] == "\\x00"


It would be better if you instanciate the headers here (from cqcHeader) and check the command w.r.t the variable in the same file. Otherwise, if we cange the headers we also need to change these tests.

Maybe with these tests it would be better to just use and read the binary files. In that case it's much easier to extract the numbers. With the text file it would be a bit more complicated to translate "\x00" to a number of to have to struct.pack() the number, convert it to text and then compare. What do you think?

@lgvacanti I'm not sure I fully understand. I think it would still be good to instanciate the headers using cqcHeader

AckslD · 2019-11-26T15:34:27Z

cqc/pythonLib.py

@@ -380,7 +623,19 @@ def close(self, release_qubits=True):
            self.flush()

        if release_qubits:
-            self.release_all_qubits()
+            if len(self.active_qubits[:]) != 0:


Why not call release_all_qubits?

The sequence with release_all_qubits went like this:

def close(self, release_qubits=True): """Handle closing actions. Flushes remaining headers, releases all qubits, closes the connections, and removes the app ID from the used app IDs. """ # Flush all remaining commands if self._pending_headers: self.flush() if release_qubits: self.release_all_qubits() self._s.close() self._pop_app_id() self.closeClassicalServer() for name in list(self._classicalConn): self.closeClassicalChannel(name)

def release_all_qubits(self): """ Releases all qubits off this connection """ return self.release_qubits(self.active_qubits[:])

def release_qubits(self, qubits, notify=True, block=True, action=False): """ Release qubits so backend can free them up for other uses :param qubits: a list of qubits to be released :param notify: Do we wish to be notified when done. :param block: Do we want the qubit to be blocked :param action: Execute the releases recursively or sequencely """ if isinstance(qubits, qubit): qubits = [qubits] assert isinstance(qubits, list) n = len(qubits) if n == 0: # then we don't need to do anything return logging.debug("App {} tells CQC: Release {} qubits".format(self.name, n)) if action: hdr_length = CQCCmdHeader.HDR_LENGTH + CQCSequenceHeader.HDR_LENGTH else: hdr_length = CQCCmdHeader.HDR_LENGTH hdr = CQCHeader() hdr.setVals(CQC_VERSION, CQC_TP_COMMAND, self._appID, hdr_length * n) cqc_msg = hdr.pack() release_messages = b"" for i in range(n): q = qubits[i] try: q.check_active() except QubitNotActiveError as e: raise QubitNotActiveError( str(e) + ". Qubit {} is not active. None of the qubits are released".format(q._qID) ) q._set_active(False) cmd_hdr = CQCCmdHeader() cmd_hdr.setVals(q._qID, CQC_CMD_RELEASE, int(notify), int(block), int(action)) release_messages += cmd_hdr.pack() if action: seq_hdr = CQCSequenceHeader() # After this one we are sending n-i-1 more releases seq_hdr.setVals(hdr_length * (n - i - 1)) release_messages += seq_hdr.pack() self._s.send(cqc_msg + release_messages) if notify: msg = self.readMessage() self.check_error(msg[0]) if msg[0].tp != CQC_TP_DONE: raise CQCUnsuppError( "Unexpected message sent back from the server. Message: {}".format(msg[0].printable()) ) self.print_CQC_msg(msg)

So release_all_qubits() isn't really needed, since you might as well just do release_qubits(self.active_qubits[:])

release_qubits() has been replaced by construct_release(), commit(), and the if notify: block. This fits better in the model of first constructing a message and then sending it.

AckslD · 2019-11-26T15:36:00Z

cqc/pythonLib.py

+        self.commit(msg)
+
+    @abstractmethod
+    def new_qubitID(self):


Both here and in the subclass it is not fully clear what this method is supposed to do. I think it would be good to have a bit more explanation in the doc-string.

I've updated them. See this commit 0a292d2

AckslD · 2019-11-26T15:38:53Z

cqc/pythonLib.py

@@ -218,6 +467,9 @@ def __init__(self, name, socket_address=None, appID=None, pend_messages=False,
            :param network_name: None or str
                Used if simulaqron is used to load socket addresses for the backend
        """
+
+        super().__init__(pend_messages=pend_messages)
+


The setting of self._opened_with_with below should probably move to the abstract class since it it checked when creating a qubit.

Done 89842ae

AckslD · 2019-11-26T15:40:53Z

cqc/pythonLib.py

+class CQCToFile(CQCHandler):
+    """Handler to be used when writing the CQC commands to a file."""
+
+    def __init__(self, filename='CQC_File', pend_messages=False,


I think we should also provide the option to specify the binary file path. Or maybe there can be an argument for if we should output text or binary and there is only one file. Also, I think the argument should instead be filepath such that the user can specify a file in another folder.

I have added an argument where you choose to have binary output or text, so there's only one file.

Still to do: absolute path instead of relative.

Done 8e0e607

AckslD · 2019-11-26T15:42:31Z

cqc/pythonLib.py

+
+        if release_qubits and self.active_qubits:
+
+            msg = self.construct_release(self.active_qubits[:], notify=True, 


I think some of these calls can be unified in this class and CQCConnection.

I changed the check for active qubits in CQCConnection to be the same as here. 9b094c5

But as for unifying, CQCConnection has a if notify: block which this doesn't. Also in CQCConnection's close() the classical servers also get closed

AckslD · 2019-11-26T15:43:36Z

cqc/pythonLib.py

+                                         block=True, action=False)
+            self.commit(msg)
+
+    def createEPR(self, name, remote_appID=0, notify=True, block=True):


Same comment as above, I think we don't need to have this logic in both of the subclasses. Or maybe we do but I cannot see why at this point

Do you mean the whole createEPR method?

Yeah, for createEPR, recvEPR, sendQubit, recvQubit and flush_factory the code is mostly the same for the two classes. It would be better to avoid this.

This is more consistent with how the rest of Python handles specifying files. You can specify an absolute or relative path.

lgvacanti · 2019-11-29T13:47:18Z

@AckslD You can take another look at this. For all your comments I have either solved them or left a question/remark for you to look at.

AckslD reviewed Oct 16, 2019

View reviewed changes

Add functionality for writing to file

2e62090

- Add CQCHandler abstract class - Make CQCConnection a subclass of CQCHandler - Add CQCWriteToFile class, which is a subclass of CQCHandler - Add tests for CQCWriteToFile

lgvacanti force-pushed the develop-CQCConnection-replace branch from 30c3137 to 2e62090 Compare October 31, 2019 15:29

lgvacanti and others added 9 commits October 31, 2019 16:48

Put str() around tmpdir, which should avoid error on python 3.5

324a82c

Linting fixes

1aad30d

Add change to stop linting warning

cbee48e

Write binary file and don't write None when releasing with no active …

ca0eb6f

…qubits

Remove bad test

3b9ba8b

Merge branch 'Develop' into develop-CQCConnection-replace

9d7980d

Note: In Develop, when creating a new qubit it send CQC Header and Command Header seperately. Here it sends them together. So a test needed to be changed to reflect that.

Add flush-on-exit to CQCToFile

b77ac14

Add test to check flush-on-exit for CQCToFile

dec78aa

Fix linting errors

e098800

lgvacanti added 7 commits November 18, 2019 11:47

Merging cqc_conditional tests and docs changes

b53f057

Start on CQCToFile docs

f82ff46

CQCToFile docs

18270d9

Add CQCToFile to rest of docs structure

c45e337

Correct some methods in CQCToFile when pending messages

7795b26

Use new_qubitID() instead of reading message

2fb5efd

Add missing option to new_qubitID

7c23cef

AckslD reviewed Nov 26, 2019

View reviewed changes

lgvacanti added 8 commits November 28, 2019 15:20

Add to what CQCToFile does not support

43f87a6

Update new_qubitID() docstrings

0a292d2

Move initial setting of _opened_with_with flag to CQCHandler

89842ae

Make check for active qubits nicer

9b094c5

Replace left over use of release_qubits()

16a5e69

Output only one file, type specified in arguments

cb854af

Fix binary always being True and fix tests

73ffc04

Change to using file instead of filename

8e0e607

This is more consistent with how the rest of Python handles specifying files. You can specify an absolute or relative path.

AckslD merged commit 8e0e607 into SoftwareQuTech:Develop Feb 23, 2020


		if release_qubits and self.active_qubits:

		msg = self.construct_release(self.active_qubits[:], notify=True,

Add CQCHandler abstract class, make CQCConnection subclass of it, add CQCToFile class for writing commands to file #39

Add CQCHandler abstract class, make CQCConnection subclass of it, add CQCToFile class for writing commands to file #39

Conversation

lgvacanti commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AckslD commented Oct 16, 2019

lgvacanti commented Oct 16, 2019

AckslD commented Oct 16, 2019 • edited Loading

lgvacanti commented Oct 16, 2019

lgvacanti commented Oct 16, 2019

lgvacanti commented Oct 17, 2019

AckslD commented Oct 17, 2019

lgvacanti commented Oct 17, 2019

lgvacanti commented Nov 13, 2019

AckslD commented Nov 15, 2019

lgvacanti commented Nov 20, 2019

AckslD left a comment • 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

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

lgvacanti commented Nov 29, 2019

AckslD commented Oct 16, 2019 •

edited

Loading

AckslD left a comment •

edited

Loading