On Tue, Jul 26, 2022 at 09:35:30AM -0400, John Kacur wrote:
> The purpose is to remove functions out of misc and use the ones in the
> file systopolgy.py
>
> - Add function collapse_cpulist(cpulist) outside of the CpuList
> class
> - Make methods longest_sequence and expand_cpulist accesible outside of
> their class
> - Add function online_cpus(self) to class SysTopology
> - Add function online_cpus_str(self) to class SysTopology
> - Add function invert_cpulist(self, cpulist) to class SysTopology
> - Convert strings to f-strings for better readability
> - Add a few missing docstrings to methods / functions, module etc
> - Add a few more tests to the unit test
>
> TODO: CpuList is suited for use by SysTopology, but is not ideal for a
> generic CpuList. A more generally usable CpuList should be created
>
> Signed-off-by: John Kacur <jkacur@xxxxxxxxxx>
> ---
> rteval/systopology.py | 90 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 74 insertions(+), 16 deletions(-)
>
> diff --git a/rteval/systopology.py b/rteval/systopology.py
> index e852f86e450f..ce8d02cf7318 100644
> --- a/rteval/systopology.py
> +++ b/rteval/systopology.py
> @@ -23,11 +23,32 @@
> # including keys needed to generate an equivalently functional executable
> # are deemed to be part of the source code.
> #
> +""" Module for querying cpu cores and nodes """
>
> import os
> import os.path
> import glob
>
> +# Utility version of collapse_cpulist that doesn't require a CpuList object
> +def collapse_cpulist(cpulist):
> + """ Collapse a list of cpu numbers into a string range
> + of cpus (e.g. 0-5, 7, 9) """
> + if len(cpulist) == 0:
> + return ""
> + idx = CpuList.longest_sequence(cpulist)
> + if idx == 0:
> + seq = str(cpulist[0])
> + else:
> + if idx == 1:
> + seq = f"{cpulist[0]},{cpulist[idx]}"
> + else:
> + seq = f"{cpulist[0]}-{cpulist[idx]}"
> +
> + rest = collapse_cpulist(cpulist[idx+1:])
> + if rest == "":
> + return seq
> + return ",".join((seq, rest))
> +
> def sysread(path, obj):
> """ Helper function for reading system files """
> with open(os.path.join(path, obj), "r") as fp:
> @@ -46,7 +67,7 @@ class CpuList:
> if isinstance(cpulist, list):
> self.cpulist = cpulist
> elif isinstance(cpulist, str):
> - self.cpulist = self.__expand_cpulist(cpulist)
> + self.cpulist = self.expand_cpulist(cpulist)
> self.cpulist = self.online_cpulist(self.cpulist)
> self.cpulist.sort()
>
> @@ -67,7 +88,7 @@ class CpuList:
> return False
>
> @staticmethod
> - def __longest_sequence(cpulist):
> + def longest_sequence(cpulist):
> """ return index of last element of a sequence that steps by one """
> lim = len(cpulist)
> for idx, _ in enumerate(cpulist):
> @@ -83,14 +104,14 @@ class CpuList:
> """
> if len(cpulist) == 0:
> return ""
> - idx = self.__longest_sequence(cpulist)
> + idx = self.longest_sequence(cpulist)
> if idx == 0:
> seq = str(cpulist[0])
> else:
> if idx == 1:
> - seq = "%d,%d" % (cpulist[0], cpulist[idx])
> + seq = f"{cpulist[0]},{cpulist[idx]}"
> else:
> - seq = "%d-%d" % (cpulist[0], cpulist[idx])
> + seq = f"{cpulist[0]}-{cpulist[idx]}"
>
> rest = self.__collapse_cpulist(cpulist[idx+1:])
> if rest == "":
> @@ -98,7 +119,14 @@ class CpuList:
> return ",".join((seq, rest))
>
> @staticmethod
> - def __expand_cpulist(cpulist):
> + def compress_cpulist(cpulist):
> + """ return a string representation of cpulist """
> + if isinstance(cpulist[0], int):
> + return ",".join(str(e) for e in cpulist)
> + return ",".join(cpulist)
> +
> + @staticmethod
> + def expand_cpulist(cpulist):
> """ expand a range string into an array of cpu numbers
> don't error check against online cpus
> """
> @@ -124,8 +152,8 @@ class CpuList:
> def is_online(self, n):
> """ check whether cpu n is online """
> if n not in self.cpulist:
> - raise RuntimeError("invalid cpu number %d" % n)
> - path = os.path.join(CpuList.cpupath, 'cpu%d' % n)
> + raise RuntimeError(f"invalid cpu number {n}")
> + path = os.path.join(CpuList.cpupath, f'cpu{n}')
>
> # Some hardware doesn't allow cpu0 to be turned off
> if not os.path.exists(path + '/online') and n == 0:
> @@ -240,8 +268,8 @@ class SysTopology:
> return len(list(self.nodes.keys()))
>
> def __str__(self):
> - s = "%d node system" % len(list(self.nodes.keys()))
> - s += " (%d cores per node)" % (len(self.nodes[list(self.nodes.keys())[0]]))
> + s = f"{len(list(self.nodes.keys()))} node system "
> + s += f"({(len(self.nodes[list(self.nodes.keys())[0]]))} cores per node)"
> return s
>
> def __contains__(self, node):
> @@ -268,6 +296,7 @@ class SysTopology:
> return n
>
> def getinfo(self):
> + """ Initialize class Systopology """
> nodes = glob.glob(os.path.join(SysTopology.nodepath, 'node[0-9]*'))
> if nodes:
> nodes.sort()
> @@ -278,27 +307,56 @@ class SysTopology:
> self.nodes[0] = SimNumaNode()
>
> def getnodes(self):
> + """ return a list of nodes """
> return list(self.nodes.keys())
>
> def getcpus(self, node):
> + """ return a dictionary of cpus keyed with the node """
> return self.nodes[node].getcpulist()
>
> + def online_cpus(self):
> + """ return a list of integers of all online cpus """
> + cpulist = []
> + for n in self.nodes:
> + cpulist += self.getcpus(n)
> + cpulist.sort()
> + return cpulist
> +
> + def online_cpus_str(self):
> + """ return a list of strings of numbers of all online cpus """
> + cpulist = [str(cpu) for cpu in self.online_cpus()]
> + return cpulist
> +
> + def invert_cpulist(self, cpulist):
> + """ return a list of online cpus not in cpulist """
> + return [c for c in self.online_cpus_str() if c not in cpulist]
>
> if __name__ == "__main__":
>
> def unit_test():
> + """ unit test, run python rteval/systopology.py """
> s = SysTopology()
> print(s)
> - print("number of nodes: %d" % len(s))
> + print(f"number of nodes: {len(s)}")
> for n in s:
> - print("node[%d]: %s" % (n.nodeid, n))
> - print("system has numa node 0: %s" % (0 in s))
> - print("system has numa node 2: %s" % (2 in s))
> - print("system has numa node 24: %s" % (24 in s))
> + print(f"node[{n.nodeid}]: {n}")
> + print(f"system has numa node 0: {0 in s}")
> + print(f"system has numa node 2: {2 in s}")
> + print(f"system has numa node 24: {24 in s}")
>
> cpus = {}
> + print(f"nodes = {s.getnodes()}")
> for node in s.getnodes():
> cpus[node] = s.getcpus(int(node))
> - print(f'cpus = {cpus}')
> + print(f'cpus = {cpus}')
> +
> + onlcpus = s.online_cpus()
> + print(f'onlcpus = {onlcpus}')
> + onlcpus = collapse_cpulist(onlcpus)
> + print(f'onlcpus = {onlcpus}')
> +
> + onlcpus_str = s.online_cpus_str()
> + print(f'onlcpus_str = {onlcpus_str}')
>
> + print(f"invert of [ 2, 4, 5 ] = {s.invert_cpulist([2, 3, 4])}")
> unit_test()
> --
> 2.37.1
>
collapse_cpulist():
- Ordered numerical lists work properly [1,2,3,4,5,10,11,12,15,18,19,20] => '1-5,10-12,15,18-20'
- Empty list works as expected [] => ''
- Non-numerical elements result in ValueError ["a", 2, 3] => ValueError
- Mixture of numerical interger and string elements works properly ["0",2,3,4] => '0,2-4'
- Unordered list does not produce range. [0,10,11,12,2,5,4,3] => '0,10-12,2,5,4,3'
Consider sorting the list before operating on it.
- Providing string to function that expects a list results in strange behavior '123' => '1-3'
Perhaps check for whether a list was provided prior to operating on input
CpuList.compress_cpulist():
- Does not accept empty list [] => IndexError.
Consider handling this case.
- Compress_cpulist is a bit misleading for a function that returns a string representation, and could be confused with collapse_cpulist.
Consider renaming to something the indicates str output.
SysTopology.online_cpus():
- Tested by intializing SysTopology object, calling online_cpus showed [0,1,2,3,4,5,6,7],
then disabled cpu 4 and tested again by calling online_cpus(), which still showed cpu 4 as active.
Reinitializing the class and calling online_cpus() worked properly and showed [1,2,3,5,6,7] where cpu 4 was disabled.
Consider adding a comment to indicate that this function displays the state of the system at class initialization.
In the future, a function may be added to display the state of the system at function call time.
>>> top = st.SysTopology()
>>> top.online_cpus()
[0, 1, 2, 3, 4, 5, 6, 7]
$ su -c 'echo 0 > /sys/devices/system/cpu/cpu4/online '
>>> top.online_cpus()
[0, 1, 2, 3, 4, 5, 6, 7]
>>> top = st.SysTopology()
>>> top.online_cpus()
[0, 1, 2, 3, 5, 6, 7]
