Store limited number of models and clear older ones

[MukuFlash03]

Clearing or trimming number of models for a particular user from the Stage_updateable_models collection.
Constant K_MODELS_COUNT defines the number of models to keep.
Comparing with write_ts of Kth model entry to decide how many models to remove which are older than this write_ts.

Currently tested with model data available in stage dataset snapshot.
DONE - Testing done by generating mock models and cleaning them up.

[shankari]

I am stopping at this point because there are multiple comments related to , please clean up this PR and re-submit for review.

[shankari]

Looks good at a high level; but I would like some polishing changes...

[shankari]

Where in here are you checking that the models that are deleted are the oldest ones? I see that the code does that with the sort on the metadata.write_ts but I don't see a test for that. If I changed

edb.get_model_db().find(find_query).sort("metadata.write_ts", -1)

to

edb.get_model_db().find(find_query).sort("metadata.write_ts", 1)

I believe the test would still pass

[MukuFlash03]

Where in here are you checking that the models that are deleted are the oldest ones? I see that the code does that with the sort on the metadata.write_ts but I don't see a test for that. If I changed

edb.get_model_db().find(find_query).sort("metadata.write_ts", -1)

to

edb.get_model_db().find(find_query).sort("metadata.write_ts", 1)

I believe the test would still pass

Thank you for pointing that out; that is indeed an important test I missed.

For testing purposes, I did change the sort(-1) to sort(1) which leads to ascending order of queried models based on write_ts.
I had the following observations :

• Initial count assertion itself fails (which is good!) as all available models satisfy write_ts condition of $ lte < write_ts_limit .

            filter_clause = {
                "user_id" : self.user_id,
                "metadata.key" : key,
                "metadata.write_ts" : { "$lte" : write_ts_limit }
            }

• Hence every time all models are being deleted and only one model remains (current_model_count = 1) which is the latest one being inserted.

if i <= (maximum_stored_model_count - 1):
    self.assertEqual(current_model_count, i+1)
else:
    self.assertEqual(current_model_count, maximum_stored_model_count)

• Just to get beyond this point, I tried testing without this assertion, to observe write_ts_lists:
  • For every (maximum_stored_model_count + 1)th model added, the first maximum_stored_model_count models are deleted and the latest model is inserted and this repeats.

---

So, to reinforce the testing, I’m adding another assertion to confirm that only the latest top K or max_model_count models are stored by storing the write_ts times in two different lists and checking for equality of the top K entries:

- model_creation_write_ts_list : stores write_ts times each time a model is created in the for loop.
- stored_model_write_ts_list : stores write_ts times of all the already stored models in the DB, which should just have the latest models.
- The last 'maximum_stored_model_count' in model_creation_write_ts_list should match those in stored_model_write_ts_list.

[shankari]

I do not 100% understand how the first assertion tripped, but I am satisfied with the changes and am happy to merge!