[bug] Device subclass without BLACS_connection may crash the compilation producing a truncated h5 file

[YinFengQi]

Issue Description

When defining a Device subclass that lacks the BLACS_connection attribute, compilation of .py files containing only such devices results in a .h5 file containing only a globals group, no addtional groups are created and no exceptions are raised during the process.

Minimal Reproduction

My environment is:

PS C:\Users\Ningy> python
Python 3.11.4 (tags/v3.11.4:d2340ef, Jun  7 2023, 05:45:37) [MSC v.1934 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>>

Following codes generates a out.h5 with the only group globals:

from labscript import Device
from labscript import start, stop
from labscript import labscript_init

class MyDevice(Device):
    # BLACS_connection = "a"
    pass

labscript_init("./out.h5", new=True, overwrite=True)

MyDevice("b", parent_device=None, connection="c")

start()

stop(1)

If the comment is removed, the output file out.h5 would behave normally.

Cause

Basically, in this bug, labscript passed a problematic arguement to numpy function empty, and then Python was terminated silently inside np.empty

Numpy Side

Following codes would terminate Python without any exception:

from numpy import *
empty(1, [
    ("foo", "a1"),
    ("bar", "a-1")
])

That seems to be a bug for numpy itself, I guess.

Labscript Side

The bug exists in generate_connection_table function in labscript.py, which goes like

max_BLACS_conn_length = -1

for device in compiler.inventory:
    if hasattr(device,"BLACS_connection"):
        BLACS_connection = str(device.BLACS_connection)
        if len(BLACS_connection) > max_BLACS_conn_length:
            max_BLACS_conn_length = len(BLACS_connection)
    else:
        BLACS_connection = ""

connection_table_dtypes = [('name','a256'), ('class','a256'), ('parent','a256'), ('parent port','a256'),
                               ('unit conversion class','a256'), ('unit conversion params', vlenstring),
                               ('BLACS_connection','a'+str(max_BLACS_conn_length)),
                               ('properties', vlenstring)]
connection_table_array = empty(len(connection_table),dtype=connection_table_dtypes)

It can be checked that if no device has the attribute BLACS_connection, the variable max_BLACS_conn_length will never be modified so it keeps the default value -1 going into np.empty and python will quit inside this function.

In standard practise, runmanager's engage buttom would send the file name to a worker process, in which compile and exec would be called successively and eventually go into stop -> generate_code -> generate_connection_table.

Expected Behavior

If no device has the BLACS_connection attribute, max_BLACS_conn_length should default to 0 or another valid value, preventing np.empty from being called with invalid arguments.

[YinFengQi]

I just checked that the issue with numpy has been fixed since numpy 2.0.0

[dihm]

@YinFengQi Thank you for the extremely detailed report! It makes my job much, much easier.

You have found a fairly subtle issue, though I am curious why you might want to create devices without BLACS_connection defined? I'd call it an anti-pattern since using labscript implies running things through BLACS, and all devices in BLACS are assumed to have a BLACS_connection property. Of course, as you have discovered, that isn't enforced and each device is welcome to use this variable however they choose, so fair enough.

This is certainly a result of labscript's long and winding history of development since the Device base class should define this attribute if we are going to insist on its presence. But I'm not inclined to make that large a change right now. The fix you suggest seems like the best way forward for now.

[YinFengQi]

I was just writing some toy class to learn how to add new devices, and I noticed that the Runmanager’s abort button doesn’t turn gray after engaging. Sometimes the GUI even freezes.🙃

Also, since low-level input/output classes also inherit from Device, wouldn’t it be less ideal to have BLACS_connection as an attribute of Device?🤔