Fix "Reload Libraries macro issues"

[PietroCampana]

This pull request addresses issue #89.
To start, I implemented a basic solution by making extract_pcell_data_from_views work only with instances of Element and by restoring the view in case of an exception when reloading the libraries.

[cla-bot]

Thank you for your pull request and welcome to our community.

We require contributors to sign our Contributor License Agreement (CLA), and it seems that the following contributors have not yet signed it: @PietroCampana.

In order for us to review and merge your code, please sign the CLA at meetiqm.com.

Once you have signed the CLA, write a comment here and we'll verify. Please note that it may take some time for us to verify it.

[PietroCampana]

I have now signed the CLA.

[cla-bot]

Thank you for your pull request and welcome to our community.

We require contributors to sign our Contributor License Agreement (CLA), and it seems that the following contributors have not yet signed it: @PietroCampana.

In order for us to review and merge your code, please sign the CLA at meetiqm.com.

Once you have signed the CLA, write a comment here and we'll verify. Please note that it may take some time for us to verify it.

[PietroCampana]

I have added the remove_kqc_pcells function to separate the deletion of PCells instances from the collection of their data. This requires to iterate over all the views and PCells one more time, but if errors occur when reloading the libraries the current view is kept instead of trying to redraw the PCells with possibly uninitialized libraries.

[qpavsmi]

Thanks for the pull request. Unfortunately there is a hidden bug in this approach, that can be replicated easily:

• Open KLayout, place some element, like double pads
• Open code, modify the placed element, for example make _build_island2 return pya.Region()
• Run Reload Libraries
• Choose the Move tool, click the element once, and click another time to some other place in the layout to move it out of the way. You'll see there were actually two elements stacked on top of each other, one with pre-change geometry and other post-change.

I believe what is happening is that after load_libraries is called, the element placed on layout stops being a subclass of KQC Element. Hence when we attempt to clean it up with remove_kqc_pcells, it does not register as KQC Element and is left alone. While also restore_pcells_to_views will place an updated copy of it on top of it.

While it's probably good that inst.delete is not called during extract_pcell_data_from_views, I think they still should be collected into some list, so that if load_libraries does not cause errors, the list is iterated to remove old version elements.

[qpavsmi]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[qpavsmi]

Another thing I recommend to test is to induce a syntax error to test that the try block for reload_libraries picks up the error correctly. If for example you modify the start of _build_island2 function to be:

    def _build_island2(self, squid_height, taper_height):
        if True:

        island2_top = self.squid_offset - squid_height / 2
        island2_polygon = pya.DPolygon(
            [
            ...

the if True: is not indented correctly, which will cause your error to show. From my tests what happens is that I no longer become able to choose "Qubit Library" since KQC code is broken, but when I "fix" the code error and run Reload Libraries again, the "Qubit Library" panel becomes available again. It would be nice if the qubit I placed on the layout becomes modifiable again after the code errors are fixed, this is something that doesn't happen in this implementation.

Another quirk is that the newline after if True: is quite important for some reason. If this is removed, running Reload Libraries causes KLayout to crash without any messages. Restarting KLayout and looking at File > Log Viewer will print some unrelated errors. I think for the purposes of the hackathon we can leave such weird bugs unaddressed if they are hard to debug.

[PietroCampana]

I have written a partial solution addressing both points. To solve the problem of duplicated instances, they are now collected at the same time of the view data in the extract_pcell_data_from_view function and deleted later. In this way every PCell that is drawn again gets deleted, leaving no duplicates.

Making the PCell modifiable again after fixing the libraries in case of any error seems to be fairly more complex, but I found a possible simple solution by moving the call to _get_all_pcell_classes at the beginning of load_libraries, before the libraries are deleted. Since the modules are actually reloaded in _get_all_pcell_classes, in this way syntax errors in the components definitions stop the function before deleting the libraries, and the KQCircuits PCells remain Element instances, which will be tracked until the error is fixed. However, in the case of errors appearing when rebuilding the libraries, the original ones will already have been deleted, losing the Element instances. At the moment the only way to keep track of the cells even when libraries are completely deleted that I can think about is to save the original PCell data collected in extract_pcell_data_from_view into a temporary file, for example by pickling them.

Regarding the last point, I couldn't manage to reproduce the quirk due to the newline after is True. On my installations (Arch linux and Ubuntu) it either generates additional error messages in the log or doesn't change behavior at all, but Klayout doesn't crash. I will send more detailed specs if you want to investigate this further.

[qpavsmi]

Thank you for your follow-up. Right now this seems to work fantastic! If I place an element, induce some code errors, reload libraries, it gives me errors and then lets me modify parameters for the element based on the previous version of the code. I only tested on Windows so far, does it perform differently on Ubuntu/Arch? Because as it works now on Windows I don't think we need to concern ourselves with temporary files at all.

The odd thing with newline after wrong indentation seems to be Windows specific. I'd say we leave this out as out-of-scope for this issue.

I'll test this on other platforms after the weekend, but I'd say in this current version this could be merged already. But I would recommend to try out the two points I brought up earlier:

• except Exception as e: Would add more transparency to the user if the error message encountered when loading code from KQC was somehow displayed, unless the spam is too much
• Is there a way that instance deletion and restore_pcells_to_views could be skipped if an error is encountered? If its more convenient to delete instances no matter what the outcome of load_libraries, that's fine by me too as a first version of this feature.

[PietroCampana]

Thank you for the nice comments. I have written a more informative error message and made some minor modifications. I found out that it was necessary to delete the cells before reloading the libraries, otherwise they would lose the references to their children's instances, which didn't get deleted and appeared out of the top cell afterwards.

The current behavior on Linux is:

• If an error appears when reloading the modules, the PCells are still drawn and can be modified according to the current library. Changes are tracked after correcting the errors.
• If the modules can be reloaded but an exception is raised when drawing a PCell, it doesn't appear on screen and the error registers only on the log, but the PCell is still retrieved and drawn correctly once the error is fixed.
• The PCells hierarchy with nested components is maintained inside of the top cell.
• No duplicates are generated.
• PCells from other libraries can be added whenever and are not considered.

Current limitations:

• The PCells are tracked for every view, but only inside of the current top cell in each of them, while the PCells in the inactive ones stop being Elements after reloading and cannot be modified according to the libraries.
• If the modules are reloaded successfully but something happens when rebuilding the libraries, it is possible that some PCells won't be tracked anymore.

[qpavsmi]

This is now in a state where I can happily merge this to our main branch! Thank you for your collaboration!