Remaining pyside qobjects use consistent shared updates[CPP-687]

[john-michaelburke]

Converted all of the remaining tabs to use the consistent shared updates approach pitched in this PR:
#447

Also moved the "data_model" for sending data from frontend to backend to its own file to appease the linter's max 1k lines per file requirement.

There were a few files we had to add more than a single container to follow the paradigm, an example of how this was done throughout all cases can be found in swiftnav_console/connection.py.

The Observation tab strictly requires the reuse of the table model which would not work for our paradigm of "singleton" usage so two child classes were made Remote/Local to stick to the paradigm.

[silverjam]

Logged a few suggestions, but overall looks good

[notoriaga]

Why do we have a bunch of these single element lists for each tab? Like for example couldn't this just be

STATUS_BAR: Dict[str, Any] = status_bar_update()

?

[john-michaelburke]

To be honest, this was initially pitched by Jason so he may be able to explain it better than me guessing. We are calling this function in a separate thread throughout the app. Ex:

@classmethod
    def post_data_update(cls, update_data: Dict[str, Any]) -> None:
        TRACKING_SKY_PLOT_TAB[0] = update_data
        cls._instance._data_updated.emit()

And then accessing the data in pyside/qml qobject land. Ex:

@Slot()  # type: ignore
    def handle_data_updated(self) -> None:
        self._tracking_sky_plot = TRACKING_SKY_PLOT_TAB[0]

I guess this in ensures there is always a valid reference to the parent list which could have some implications with respect to garbage collection. But not sure if this is technically needed?

Probably need @silverjam to explain 😄

[silverjam]

@notoriaga It's using a list as a "pointer" so we can modify a global variable without using a global statement -- was also thinking that for some of these updates we might need to eventually replace these with a queue.Queue or use list.pop(0) and list.push(item) to insert/remove data updates.

[notoriaga]

Ahh I see, makes sense!

[samvrlewis]

It's been a little while since I've looked at the console, and to be honest I'm not sure I ever fully understood the data update model, so I've written up my understanding as a precursor to my question just in case my understanding is incorrect!

Tab State

Each tab holds state in the way of a global list that contains a dictionary. This represents the latest data that has arrived from the backend.

Eg:

ADVANCED_IMU_TAB = [
    {
        "FIELDS": [],
        "POINTS": [],
    }
]

Tabs generally have a QObject class that holds a reference to this latest data from the backend, eg:

class AdvancedImuPoints(QObject):
    advanced_imu_tab: Dict[str, Any] = {}
    def __init__(self):
        self.advanced_imu_tab = ADVANCED_IMU_TAB[0]

To interact with this data from the QML code, there are models registered that allow the QML to control when data is read out from the global state into the class, eg:

class AdvancedImuModel(QObject):
    @Slot(AdvancedImuPoints)
    def fill_console_points(self, cp: AdvancedImuPoints) -> AdvancedImuPoints:
        cp.set_points(cp.advanced_imu_tab[Keys.POINTS])
        cp.set_fields_data(cp.advanced_imu_tab[Keys.FIELDS_DATA])
        return cp

After calling the above from QML, the QML code can then access points and fields from the getters on the original QObject. My assumption is this is to make it safe for the QML to access data which might be changed from under it?

The QML calls this code to transfer data out from the global state on a timer, if the tab is active.

Messages

receive_messages is running in a QThread and polls for messages from the backend. When that thread receives a message from the backend, it reaches into the global state for that tab and updates it, eg:

# in receive_messages
elif m.which == Message.Union.AdvancedImuStatus:
    advanced_imu_tab = advanced_imu_tab_update() # a new dictionary
    advanced_imu_tab[Keys.FIELDS_DATA][:] = m.advancedImuStatus.fieldsData
    advanced_imu_tab[Keys.POINTS][:] = [
        [QPointF(point.x, point.y) for point in m.advancedImuStatus.data[idx]]
        for idx in range(len(m.advancedImuStatus.data))
    ]
    AdvancedImuPoints.post_data_update(advanced_imu_tab)

# in the IMU tab class
class AdvancedImuPoints(QObject):
    @classmethod
    def post_data_update(cls, update_data: Dict[str, Any]) -> None:
        ADVANCED_IMU_TAB[0] = update_data
        cls._instance._data_updated.emit()

Question

So this leads me onto my question - can there be a data race between the QML code calling in and receive_messages updating the global state? ie (in the IMU example) between fill_console_points and AdvancedImuPoints.post_data_update ? From what I understand of python and the GIL it should be safe for concurrent read/writes on the global state dict (though I'm not sure how QML effects that..?) but maybe what's not safe is if something like this happens:

    def fill_console_points(self, cp: AdvancedImuPoints) -> AdvancedImuPoints:
        cp.set_points(cp.advanced_imu_tab[Keys.POINTS])
        # post_data_update called here in the other thread
        cp.set_fields_data(cp.advanced_imu_tab[Keys.FIELDS_DATA])
        return cp

Which would make AdvancedImuPoints.points and AdvancedImuPoints.fields inconsistent? In this particular case I don't think it would matter too much but in other tabs maybe it does? The tracking_signals_tab looks like it does a fair bit on update for example. Would it be better/safer to lock access to the global state so we can't have these sort of inconsistencies?

I guess this question maybe isn't that relevant to this particular PR as it looks like we've always been running receive_messages in a different thread but was really my own concern when trying to understand this change - the actual change looks good to me!

[silverjam]

This is a great write-up, we probably should've done this kind of write-up for the PR itself, just to explain what was going on.

It's been a little while since I've looked at the console, and to be honest I'm not sure I ever fully understood the data update model, so I've written up my understanding as a precursor to my question just in case my understanding is incorrect!

Tab State

Each tab holds state in the way of a global list that contains a dictionary. This represents the latest data that has arrived from the backend.

Eg:

ADVANCED_IMU_TAB = [
    {
        "FIELDS": [],
        "POINTS": [],
    }
]

Tabs generally have a QObject class that holds a reference to this latest data from the backend, eg:

These global dictionaries are kind of hold-overs from a very early prototype of the console, in many cases we could probably eliminate them by moving data storage in the QObject models themselves (assuming we do it in a way that ensures consistency).

class AdvancedImuPoints(QObject):
    advanced_imu_tab: Dict[str, Any] = {}
    def __init__(self):
        self.advanced_imu_tab = ADVANCED_IMU_TAB[0]

To interact with this data from the QML code, there are models registered that allow the QML to control when data is read out from the global state into the class, eg:

class AdvancedImuModel(QObject):
    @Slot(AdvancedImuPoints)
    def fill_console_points(self, cp: AdvancedImuPoints) -> AdvancedImuPoints:
        cp.set_points(cp.advanced_imu_tab[Keys.POINTS])
        cp.set_fields_data(cp.advanced_imu_tab[Keys.FIELDS_DATA])
        return cp

Again, this is where we could probably access the model directly and we could eliminate these fill_* messages. The original prototype was fairly sloppy here, and assumed that data stored in different keys of the dictionary was not related (in later code updates we violated this assumption, but in fairness, it is indeed sloppy, and also hard to maintain). So, inconsistency between the keys could lead to the QML code doing weird things if the receive message thread was updating things while this fill_* method is running (under the old model).

As you mentioned, these fill methods are driven by timers in the Qt code. We could probably gain some performance by eliminating the fill methods (and the timers) and just letting Qt poll the data model for changes whenever it wanted to paint. This is what we do for the table model and (as far as I remember) it gained us lot of performance advantage in that tab.

After calling the above from QML, the QML code can then access points and fields from the getters on the original QObject. My assumption is this is to make it safe for the QML to access data which might be changed from under it?

The QML calls this code to transfer data out from the global state on a timer, if the tab is active.

Messages

receive_messages is running in a QThread and polls for messages from the backend. When that thread receives a message from the backend, it reaches into the global state for that tab and updates it, eg:

# in receive_messages
elif m.which == Message.Union.AdvancedImuStatus:
    advanced_imu_tab = advanced_imu_tab_update() # a new dictionary
    advanced_imu_tab[Keys.FIELDS_DATA][:] = m.advancedImuStatus.fieldsData
    advanced_imu_tab[Keys.POINTS][:] = [
        [QPointF(point.x, point.y) for point in m.advancedImuStatus.data[idx]]
        for idx in range(len(m.advancedImuStatus.data))
    ]
    AdvancedImuPoints.post_data_update(advanced_imu_tab)

The important new thing here is the post_data_update which emits a Qt signal inside the method's implementation, the signal is connected to a QObject that lives in another thread (the model, which lives in the GUI thread). Cross thread "signal delivery" in Qt automatically posts a message to the other QThread's event loop, so that the signal is delivered on the right thread (and in this case, on the GUI's event loop).

# in the IMU tab class
class AdvancedImuPoints(QObject):
    @classmethod
    def post_data_update(cls, update_data: Dict[str, Any]) -> None:
        ADVANCED_IMU_TAB[0] = update_data
        cls._instance._data_updated.emit()

Question

So this leads me onto my question - can there be a data race between the QML code calling in and receive_messages updating the global state? ie (in the IMU example) between fill_console_points and AdvancedImuPoints.post_data_update ? From what I understand of python and the GIL it should be safe for concurrent read/writes on the global state dict (though I'm not sure how QML effects that..?) but maybe what's not safe is if something like this happens:

    def fill_console_points(self, cp: AdvancedImuPoints) -> AdvancedImuPoints:
        cp.set_points(cp.advanced_imu_tab[Keys.POINTS])
        # post_data_update called here in the other thread
        cp.set_fields_data(cp.advanced_imu_tab[Keys.FIELDS_DATA])
        return cp

Which would make AdvancedImuPoints.points and AdvancedImuPoints.fields inconsistent? In this particular case I don't think it would matter too much but in other tabs maybe it does? The tracking_signals_tab looks like it does a fair bit on update for example. Would it be better/safer to lock access to the global state so we can't have these sort of inconsistencies?

The fact that we're posting a message and updating the internal reference to new copy of an "update dictionary" prevents any inconsistency. So, in theory, as long as the data is consistent and ownership is unique, we should be able to prevent data races without locks.

I guess this question maybe isn't that relevant to this particular PR as it looks like we've always been running receive_messages in a different thread but was really my own concern when trying to understand this change - the actual change looks good to me!

The global dictionaries could be inconsistent in the old model because the receive thread could be updating different keys of the dictionary and even with Python's GIL this would result in keys in inconsistent states (especially if those keys contained related data, like lists that needed to be the same length).

[samvrlewis]

Ahh that makes a lot of sense, I missed that handle_data_updated would be called in the GUI's event loop but if that's the case then I see now that there shouldn't ever been any inconsistencies between what's in the update dictionary.

Sketched out a diagram that helped me better picture this and prove to myself that it's correct as well:

[silverjam]

Ahh that makes a lot of sense, I missed that handle_data_updated would be called in the GUI's event loop but if that's the case then I see now that there shouldn't ever been any inconsistencies between what's in the update dictionary.

Sketched out a diagram that helped me better picture this and prove to myself that it's correct as well:

Nice, mind adding this to https://github.com/swift-nav/swift-toolbox/blob/main/docs/DESIGN.md?