Scalability fixes - Load model once per user

[MukuFlash03]

Implementing code changes for improving scalability as per the requirements in this issue.

Initial approach involves utilizing Singleton design pattern concept on instance variables to check whether model has already been loaded before attempting to load the model.

Thus this should prevent model from being loaded if it is already present in the instance variable of the current class instance of BuiltinModelStorage.

More details present here.

[shankari]

Needs redesign

[MukuFlash03]

[SOLVED]
Two pipeline tests were failing - TestLabeIInferencePipeline, TestExpectationPipeline.

FAIL: testPipelineIntegrity (pipelineTests.TestExpectationPipeline.TestExpectationPipeline)
web-server_1  | ----------------------------------------------------------------------
web-server_1  | Traceback (most recent call last):
web-server_1  |   File "/src/e-mission-server/emission/tests/pipelineTests/TestExpectationPipeline.py", line 76, in testPipelineIntegrity
web-server_1  |     self.assertGreaterEqual(len(self.expected_trips), 1)  # Make sure we've actually loaded trips
web-server_1  | AssertionError: 0 not greater than or equal to 1

Took a look at the logs and saw that some more functions were called on running pipeline in these tests.
Failed since they were calling placeholder_predict_n functions in eacili inferrers.py

TypeError: placeholder_predictor_3() takes 1 positional argument but 2 were given

So, added a quick fix to update the functions so that they now receive trip_list instead of single trip and pass in a list of trips.

[shankari]

future fix??: my one comment here is that if we standardize on an interface for the algorithm_fn, I believe we can simplify this further.

This can become (pseudocode):

for algorithm_id, algorithm_fn in primary_algorithms.items():
   algo_model = algorithm.load_model()
   for trip in trip_list:
      prediction = model.predict(trip)
      # and then the original code

Back when this code was written, trip_model, with its clean interfaces to load, predict and save, had not yet been implemented. So algorithm_fn was supposed to be an abstraction layer of the multiple algorithms.

But now that we have the abstractions in trip_model, I am not sure that we need two levels of abstraction. I am open to deferring the cleanup until the next round, but I am also open to just removing one level of abstraction right now, since that will result in far fewer changes.

[shankari]

Right now, there is a map that has algorithm name to algorithm fn and then the algorithm fn (in our case) calls predict_with_n. instead, the map can have algorithm name to dict of model parameters, and then inside the for loop, we can just load the model and continue to predict trip by trip and not load the model within the predict function (which works, but is not pretty).

Alternatively, we can keep the algorithm function, eacili.predict_cluster_confidence_discounting but load the model in there. But I don't see the need for the second level of abstraction if we assume that all our algorithms will follow the L -> P -> S workflow.

[shankari]

If you are not comfortable with removing the algorithm_fn abstraction, you can simply do the following 2 lines of code to be changed.

def predict_cluster_confidence_discounting(trip, max_confidence=None, first_confidence=None, confidence_multiplier=None):
    # load application config
    model_type = eamtc.get_model_type()
    model_storage = eamtc.get_model_storage()
    ### load model  <------ NEW CODE
    labels, n = eamur.predict_labels_with_n(trip, model) <----- CHANGED CODE
    if n <= 0:  # No model data or trip didn't match a cluster
        logging.debug(f"In predict_cluster_confidence_discounting: n={n}; returning as-is")
        return labels

    confidence_coeff = n_to_confidence_coeff(n, max_confidence, first_confidence, confidence_multiplier)
    logging.debug(f"In predict_cluster_confidence_discounting: n={n}; discounting with coefficient {confidence_coeff}")

    labels = copy.deepcopy(labels)
    for l in labels: l["p"] *= confidence_coeff
    return labels

[MukuFlash03]

I have a few queries which are still confusing to me.
I can go ahead with the minimal change of loading model in eaclii but wanted to at least try and understand scope and efforts on changes required in reducing abstractions before making the simpler changes.

But now that we have the abstractions in trip_model, I am not sure that we need two levels of abstraction.

1. By "two levels", I believe you are referring to:

eacili.predict_cluster_confidence_discounting -> eamur.predict_labels_with_n -> model.predict()

I see that trip_model is an abstract class and one child class / concrete implementation is GreedySimilarityBinning class.
Do we intend on removing the calls to the first two functions in the above chain and directly call only model.predict() which is in effect a call to GreedySimilarityBinning.predict() depending on the model parameters?

---

Right now, there is a map that has algorithm name to algorithm fn and then the algorithm fn (in our case) calls predict_with_n. instead, the map can have algorithm name to dict of model parameters...

Alternatively, we can keep the algorithm function, eacili.predict_cluster_confidence_discounting but load the model in there. But I don't see the need for the second level of abstraction if we assume that all our algorithms will follow the L -> P -> S workflow.

2. I believe I've understood that the same trip model will be loaded for each user irrespective of algorithm if we move up the model loading, model parameters up into eacilp.pipeline.

If this is correct, then yes, using a map of algorithm_name to dict_model_parameters is useful:

primary_algorithms = {
    ecwl.AlgorithmTypes.CONFIDENCE_DISCOUNTED_CLUSTER: dict_model_params
}

# In the for loop
   # Load model
   for trip in trip_list:
      prediction = model.predict(trip)

But in doing this (in continuation with my 1st question), this will call model.predict(), and I fail to see how our algorithm_name will have a unique implementation if we are removing the call to its respective function?
For instance, on removing the call to eacili.predict_cluster_confidence_discounting, we are no longer doing the confidence_coeffcalculations.
So what would be the significance of the algorithm name in the map in this case?

We also have placeholder_predictor() functions in inferrers.py which are being called in TestLabelInferencePipeline.py through the compute_and_save_algorithms() as well.
I believe for these functions, loading model and making predictions won't be useful as these don't load the model and just return predictions.

[shankari]

I see that you made the relevant changes - is this still confusing? If so, we can discuss in a 1:1.

[shankari]

I am going to merge this for now, but please note the "future fix" comments and address them in a cleanup pass. You should also look through all the logs that you have added and make sure that you are logging as much detail as possible, so it can actually be useful in understanding the problem.

[shankari]

nit, future fix: we should log the length of the list

[shankari]

isn't this always going to be true, given that you define inferred_trip_list in line 62?
Are you actually testing against the "no trip" case?
I suspect this doesn't matter, since we just iterate through the trips downstream, but then we can just skip this check altogether given that it will always be true.

[MukuFlash03]

I did test this out and Python evaluates an empty list to be False when used in an if condition.
So, it does not enter the if block unless trips are loaded into inferred_trip_list in the for loop above the if block and the list becomes non-empty which then evaluates to True.

[shankari]

nit, future fix: you should add some additional details to the log - what were the unique user IDs, how many is "multiiple"? Otherwise, if I see this log, I am not sure how to debug it.

[shankari]

@MukuFlash03 I am squash-merging this to avoid code churn.
Please make sure to handle branch updates accordingly.