psyinfra / HTCAnalyze

MIT License
0 stars 3 forks source link

Create resource class #118

Closed mathisloevenich closed 3 years ago

mathisloevenich commented 3 years ago

The idea is to create an instance of the Resource object that contains all the data for a resource. If you need to modify it (i.e., add a warning level for the resource, compute some derived value, etc.) you can do so inside of the Resource instance. Any modifications will then be grouped together, rather than spread out over the script.

In your current script you change the value of resources['Usage'] to a color-coded string. Now you've prevent yourself from working with the usage value, if the need ever arises. You've also changed the type of the variable from an integer (or float) to a string. Using the Resource class approach, you instead format the textual output later in the code, using the Resource.usage_level to determine the color tags.

We can discuss the relevance of this in the group meeting if you'd like.

_Originally posted by @nhjjreuter in https://github.com/psyinfra/HTCAnalyze/pull/115#discussion_r510022323_

mathisloevenich commented 3 years ago

I suggest to make a resource class for this.

class Resource(object):
    def __init__(resources: str, usage: int, requested: int, allocated: int):
        # naming in __init__ arguments should be consistent with names in the resources dictionary
        self.name = resources  
        self.usage = usage
        self.requested = requested
        self.allocated = allocated
        self.usage_level = None

def refactor_resources(resources: dict) -> List[Resource]:
    """Convert a dict of lists to a list of Resource objects"""
    resources = {k.lower(): v for k, v in resources.items()}
    resources = [dict(zip(resources, v)) for v in zip(*resources.values())]
    resources = [Resource(**resource) for resource in resources]
    return resources

Instead of passing a dictionary object to the class, you pass a list of Resource objects to the class (or, alternatively, you convert the dictionary to a list resource objects inside of this class -- whichever fits better logistically).

Then instead of n_resources = len(resources['Resources']) (and the code directly following this) you can do the following:

for resource in resources:
    if resource.requested != 0:
        deviation = resource.usage / resource.requested  # note: they become floats, no need to pre-assign them

        if 1 - self.bad_usage <= deviation <= 1 + self.bad_usage:
            resource.usage_level = 'error'
        elif 1 - self.tolerated_usage <= deviation <= 1 + self.tolerated_usage:        
            resource.usage_level = 'warning'
        elif str(usage) == 'nan':
            resource.usage_level = 'light_warning'  # or some other name to indicate usage of 'yellow2'
        else:
            resource.usage_level = 'normal'

In a later part of the code (the part that is all about providing the text output, separated from this level of logic), you can then have:

def get_color(level: str) -> str:
    """Convert an alert level to an appropriate color"""
    colors = {'error': 'red', 'warning': 'yellow', 'light_warning': 'yellow2', 'normal': 'green'}
    return colors[level]

def print_output(resources: List[Resource]) -> None:
    """This is just a dummy function to explain the printing side of things"""
    for resource in resources:
        color = get_color(resource.usage_level)
        print(f'[{color}]{resource.usage}[/{color}]')
mathisloevenich commented 3 years ago

_More discussion about this : https://github.com/psyinfra/HTCAnalyze/pull/115#discussion_r509912233_

mathisloevenich commented 3 years ago

Done