2
\$\begingroup\$

As a way to learn PySimpleGUI, and eventually tkinter, I re-implemented a computing cluster CPU load monitor I wrote long ago. I'm looking for any feedback on whether it's well-written, but in particular I have concerns about two areas:

One, whether the organizational structure is correct. As objects, Cluster contains Nodes which contains CPUs makes sense, but should they exist on the same level, or should, say, the ClusterNode class be defined inside the Cluster class?

Two, if the display logic is too tightly integrated with the objects. On the one hand, without displaying anything, the objects have no reason to exist. On the other, this could be easily implemented procedurally with less code overall.

It also probably could do with a little more commenting, but it felt like anything more I had to add was merely restating what the code does.

Thanks for any feedback.

"""
Display load of each CPU in a cluster of nodes, using PySimpleGUI.

A Cluster object contains multiple ClusterNode objects, which in turn
contains multiple NodeCPU objects.

The window layout is: [[<node1 frame>, ..., <nodeN frame>]]
which may be broken up into sublists for multiple rows (NODES_PER_ROW).
A node frame layout is: [[<cpu1 graph>, ..., <cpuN graph>]]
where each CPU graph object is a PySimpleGUI Graph (canvas).

To simulate a real cluster, the load numbers are generated randomly
and vary gradually from interval to interval.
"""

from random import randint
import PySimpleGUI as psg

NODES_PER_CLUSTER = 12
CPUS_PER_NODE = 4

NODES_PER_ROW = 4

graphsize = (30, 100)
colors = ("red", "green", "yellow", "blue")

class NodeCPU:
    """Create an CPU object and define the Graph layout"""
    def __init__(self, num):
        self.color = colors[num]
        self.graph = psg.Graph(graphsize, (0, 0), graphsize)
        self._load = 0

    @property
    def load(self):
        """Adjust the load randomly, bound between 0 and 100"""
        self._load += randint(-5, 5)
        self._load = max(0, min(self._load, 100))
        return self._load

    def update_graph(self):
        """Update the graph, shifting the display to the left"""
        self.graph.Move(-1, 0)
        self.graph.DrawLine((graphsize[0], 0), (graphsize[0], self.load),
                            width=1, color=self.color)

class ClusterNode:
    """A ClusterNode consists of CPUs and defines the frame layout"""
    def __init__(self, num):
        self.name = f"node{num+1:02d}"
        self.cpus = [NodeCPU(cpu) for cpu in range(CPUS_PER_NODE)]
        self.frame = psg.Frame(self.name,
                              [[cpu.graph for cpu in self.cpus]],
                              title_location="n")

    def __len__(self):
        return len(self.cpus)

    def __getitem__(self, index):
        return self.cpus[index]

class Cluster:
    """A Cluster consists of ClusterNodes and defines the window layout"""
    def __init__(self):
        # set global options
        psg.theme("Black")
        psg.SetOptions(element_padding=(0, 0))
        self.nodes = [ClusterNode(node) for node in range(NODES_PER_CLUSTER)]
        self.layout = [self[node].frame for node in range(NODES_PER_CLUSTER)]
        # split the layout into multiple rows
        self.layout = self.split_list(self.layout, NODES_PER_ROW)
        self.window = psg.Window("Cluster CPU Load", self.layout,
                                resizable=False, grab_anywhere=True)
        self.window.Finalize()

    def __len__(self):
        return len(self.nodes)

    def __getitem__(self, index):
        return self.nodes[index]

    @staticmethod
    def split_list(lst, elements):
        """Split a list into sublists of 'n' elements"""
        if not isinstance(elements, int) or elements < 1:
            raise ValueError("sublist size must be int > 0")
        return [lst[i:i+elements] for i in range(0, len(lst), elements)]


def main():
    clust = Cluster()

    # event loop; show half the nodes each loop for better performance
    odds = True
    while True:
        event, values = clust.window.read(timeout=75)
        if event == psg.WIN_CLOSED:
            break
        for node in clust[odds::2]:
            for cpu in node:
                cpu.update_graph()
        odds = not odds
    clust.window.close()

if __name__ == "__main__":
    main()
\$\endgroup\$
0
\$\begingroup\$

Code looks nice. I would keep classes on the same level.
It seems more readable when you can read smaller part of code.

But your code looks like mix of Cluster Simulator + GUI Monitor and in real situation they would be two separated elements. You would have some external module with classes to access "hardware" and you would create own classes with GUI - so I would split it in code to make it more real.

Class NodeCPU would use update() only to update _load and class GUINodeCPU() would keep instance of NodeCPU to get data from _load and display it in graph. Similar situation should be with other classes. All GUI would be in own classes and all "hardware" would be in own classes.

This way you could use Cluster Simulator with other GUI like tkinter, PyQt or even create Text GUI in console with curses or urwind. Or you could use Web Framework like Flask to create web GUI.


BTW: there is system monitoring program Glances created in python and it uses python module psutils for access hardware - and maybe you could use psutil in your GUI.

psutil was used in other programs - ie. facebook created osquery to access information using SQL queries.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.