Add implementation for clearing cache table #177

hjk1030 · 2024-04-29T14:34:15Z

Added a function to delete all the data in the cache table. Trying to resolve issue #122.

hjk1030 · 2024-04-29T14:39:23Z

By the way, do I need to add tests for this?

ttt-77 · 2024-04-29T14:42:15Z

By the way, do I need to add tests for this?

Yes.

hjk1030 · 2024-05-01T15:25:02Z

I modified an existing test to validate this since adding another test makes the existing test fail.

aidb_utilities/db_setup/clear_cache.py

ttt-77 · 2024-05-01T21:22:08Z

aidb_utilities/db_setup/clear_cache.py

+    for service_binding in engine._config.inference_bindings:
+        if isinstance(service_binding, CachedBoundInferenceService):
+            async with service_binding._engine.begin() as conn:
+                stmt = delete(service_binding._cache_table)


use meaningful variable names

ttt-77 · 2024-05-01T21:28:35Z

aidb_utilities/db_setup/clear_cache.py

+                await conn.execute(stmt)
+                tables = service_binding.get_tables(service_binding.binding.output_columns)
+                for table_name in tables:
+                    stmt = delete(service_binding._tables[table_name]._table)


If tables have dependencies, does deleting them in any arbitrary order cause issues?
(e.g. table B has a foreign key that refers to table A, does deleting table A first cause an issue?)

ttt-77 · 2024-05-01T21:30:25Z

tests/tests_caching_logic.py

@@ -9,6 +9,8 @@
 from tests.inference_service_utils.inference_service_setup import register_inference_services
 from tests.inference_service_utils.http_inference_service_setup import run_server
 from tests.utils import setup_gt_and_aidb_engine, setup_test_logger
+from aidb.utils.asyncio import asyncio_run
+from aidb_utilities.db_setup.clear_cache import clear_ML_cache


ttt-77 · 2024-05-01T21:30:36Z

launch.py

@@ -8,7 +8,7 @@
 from aidb_utilities.db_setup.blob_table import BaseTablesSetup
 from aidb_utilities.db_setup.create_tables import create_output_tables
 from aidb.utils.asyncio import asyncio_run
-
+from aidb_utilities.db_setup.clear_cache import clear_ML_cache


ttt-77 · 2024-05-01T21:34:27Z

tests/tests_caching_logic.py

        aidb_res = aidb_engine.execute(aidb_query)
        assert len(gt_res) == len(aidb_res)
        # running the same query, so number of inference calls should remain same
        # temporarily commenting this out because we no longer call infer_one
-        assert aidb_engine._config.inference_services["objects00"].infer_one.calls == calls[index]
+        assert aidb_engine._config.inference_services["objects00"].infer_one.calls == calls[index][0], f"Wrong query count: Expected {calls[index][0]}, Actual {aidb_engine._config.inference_services['objects00'].infer_one.calls}"


This line is too long. Maximum line length is 80 characters.

hjk1030 · 2024-05-02T08:30:37Z

I modified the code style. Also changes the delete order to depend on foreign key references.

ttt-77 · 2024-05-03T00:18:35Z

aidb_utilities/db_setup/clear_cache.py

+  Clear the cache table at start if the ML model has changed.
+  Delete the cache table for each service.
+  '''
+  for inference_binding in engine._config.inference_bindings:


Can you refactor the code?

Collect all tables.

Construct a Table Graph

topological sort

Delete tables.
You can refer to

aidb/aidb/config/config.py

Line 65 in d8e78e4

def table_graph(self) -> Graph:

and https://github.com/ddkang/aidb/blob/d8e78e488ac91b78e8cc002b06aa01dfa8f39dfd/aidb/config/config.py#L121C23-L121C39

Ok, modified that.

hjk1030 · 2024-05-06T05:37:51Z

@ttt-77 Could you please review this PR again?

Have you added this test to Github Action?

Yes. The test is modified based on an original test and I verified it is executed.

ttt-77 · 2024-05-08T00:43:05Z

Looks good to me. @ddkang

ddkang · 2024-05-08T01:04:34Z

aidb_utilities/db_setup/clear_cache.py

+
+
+async def clear_ML_cache(engine: Engine):
+  '''


Use standard naming conventions "clear_ml_cache"

Also why is this in the DB setup and not the engine?

ddkang · 2024-05-08T01:04:56Z

aidb_utilities/db_setup/clear_cache.py

+  '''
+  Clear the cache table at start if the ML model has changed.
+  Delete the cache table for each service.
+  '''


Describe the logic in the comment

* Move the clear cache function to engine * Function name change * Add code logic comment

ddkang · 2024-05-08T04:13:38Z

tests/tests_caching_logic.py

@@ -47,24 +51,35 @@ async def test_num_infer_calls(self):
      # no service calls before executing query


We should have a separate test that tests that other ML models don't get removed when one is removed

* Add deletion for services seperately * Fix launch.py * Add test to check whether only cache for one service is deleted

* Change function param declaration to be compatible with python 3.8 * Change call count logic since all the inference service use the same counter * Add join() function to terminate the test server completely

* Clear cache before test run * Set count target corresponding to initial call count

* run cache clearing using asyncio

* Fix typo

* Add join() and sleep to make sure the server terminates

hjk1030 · 2024-05-10T00:50:17Z

@ddkang Could you please review this?

hjk1030 · 2024-05-10T00:54:18Z

tests/tests_caching_logic.py

@@ -84,6 +84,8 @@ async def test_num_infer_calls(self):
      del gt_engine
      del aidb_engine
    p.terminate()
+    p.join()


Also, I cannot terminate the test server completely without this. I'm not sure whether it's a bug or I'm not using the correct way to write multiple tests.

Please check this in depth

I believe the test server wasn't properly closed before. The server will still occupy the port if only the terminate() function is called. However most test classes only have one unit test or the service is not called, so it did not cause any problems.

ddkang · 2024-05-10T01:21:35Z

aidb/engine/engine.py

@@ -42,3 +49,53 @@ def execute(self, query: str, **kwargs):
      raise e
    finally:
      self.__del__()
+
+  async def clear_ml_cache(self, service_name_list = None):


Is there a reason this function is so complicated?

Most of this function is building the foreign key relationship graph since deleting the data referenced by another output table will cause error. I believe this is necessary unless there exists such a graph already.

Are there other functions that build the fk relationship graph? If so, that should be refactored

* Use the existing inference topological order as delete order

hjk1030 · 2024-05-14T15:50:54Z

I refactored the function using the topological order of inference services.

ddkang · 2024-05-15T04:49:16Z

@ttt-77 please check

ttt-77 · 2024-05-15T06:38:04Z

aidb/engine/engine.py

+        if isinstance(bounded_service, CachedBoundInferenceService):
+          if bounded_service.service.name in service_name_list:
+            for input_column in bounded_service.binding.input_columns:
+              service_name_list.add(input_column.split('.')[0])


why do you add table name into service_name_list?

ttt-77 · 2024-05-15T06:40:14Z

aidb/engine/engine.py

+              service_name_list.add(input_column.split('.')[0])
+            asyncio_run(conn.execute(delete(bounded_service._cache_table)))
+            for output_column in bounded_service.binding.output_columns:
+              asyncio_run(conn.execute(delete(bounded_service._tables[output_column.split('.')[0]]._table)))


The output tables may be same. Can you add them into a set and then delete them?

ttt-77 · 2024-05-15T06:48:02Z

tests/tests_caching_logic.py

+      for index, (query_type, aidb_query, exact_query) in enumerate(queries):
+        # Run the query on the aidb database
+        logger.info(f'Running query {exact_query} in ground truth database')
+        # Run the query on the ground truth database


Write comments in the correct locations.

ttt-77 · 2024-05-15T06:49:23Z

tests/tests_caching_logic.py

+      del aidb_engine
+    p.terminate()
+    p.join()
+    time.sleep(1)


Is this necessary?

I believe the join() is necessary or the server startup for the second test has some problems: https://github.com/ddkang/aidb/actions/runs/9025134318/job/24800303051. The sleep() is redundant and I have removed that.

* Fix how the service to clear cache are collected * Reduce redundant table deletion * Comment Fix * Remove redundant sleep

* Use the correct graph to collect service

* Fix

hjk1030 · 2024-05-15T17:08:48Z

@ttt-77 Could you please review this?

ttt-77 · 2024-05-16T02:25:22Z

tests/tests_caching_logic.py

+
+      asyncio_run(aidb_engine.clear_ml_cache(["lights01"]))
+
+      for index, (query_type, aidb_query, exact_query) in enumerate(queries):


Refactor the code. Could this loop be merged into previous one?

ttt-77 · 2024-05-16T02:37:55Z

aidb/engine/engine.py

+      service_name_list = set(service_name_list)
+
+      # Get all the services that need to be cleared because of foreign key constraints
+      inference_graph = self._config.inference_graph


The edge in inference_graph doesn't represent two nodes have foreign key constraints

hjk1030 · 2024-05-18T02:20:15Z

I've reconsidered the clearing logic. Now I'm planning to use the following procedure:

Collect the output tables directly related to the selected services.
Collect the output tables that need to be cleared considering the fk constraints and service constraints (if one of the output tables in a service needs to be cleared, all the tables belonging to the service have to be cleared as well). This will use the table_graph in the config, and I'll create a new map at the beginning to get all the services related to a table(I didn't find an existing map for this).
Delete the cache tables. No fk refers to the cache table so this should not cause a problem.
Delete the output tables in the reversed topological order of table_graph.

Are there any problems with this procedure? Or can it be simplified in some way?

ttt-77 · 2024-05-18T03:09:34Z

Looks good.

* Merge the two stage of testing using a loop * Refactor the cache clearing using the table graph

hjk1030 · 2024-05-19T16:15:15Z

@ttt-77 Could you please check this? The clearing steps are more complicated than I thought but I think all the steps are necessary.

Add implementation for clearing cache table

dcfe022

hjk1030 added 3 commits May 1, 2024 22:06

Add tests for cache clearing & Fix bug that does not delete output

4fbf90a

Fix wrong quotation marks used

3400098

Fix: merge tests to single test

35e5df2

hjk1030 marked this pull request as ready for review May 1, 2024 15:25

ttt-77 reviewed May 1, 2024

View reviewed changes

Fix code style & Add topological sorting for delete

c091cb5

hjk1030 requested a review from ttt-77 May 2, 2024 08:30

ttt-77 reviewed May 3, 2024

View reviewed changes

hjk1030 added 2 commits May 3, 2024 23:14

Refactor code using networkx

ccc3a73

Change order of getting tables and build graph

e24e257

hjk1030 requested a review from ttt-77 May 3, 2024 16:59

ttt-77 reviewed May 6, 2024

View reviewed changes

ddkang reviewed May 8, 2024

View reviewed changes

Modify code style

ab5fc3a

* Move the clear cache function to engine * Function name change * Add code logic comment

ddkang reviewed May 8, 2024

View reviewed changes

hjk1030 added 6 commits May 9, 2024 23:25

Adding test for seperate service cache cleaning

504da9e

* Add deletion for services seperately * Fix launch.py * Add test to check whether only cache for one service is deleted

Fix problems causing test failure

a845a00

* Change function param declaration to be compatible with python 3.8 * Change call count logic since all the inference service use the same counter * Add join() function to terminate the test server completely

Fix bugs occured in multiple tests

49ad1e1

* Clear cache before test run * Set count target corresponding to initial call count

Fix typo when clearing cache

ae8ec23

* run cache clearing using asyncio

Fix typo in engine

76d84d1

* Fix typo

Fix: close server completely

1962e02

* Add join() and sleep to make sure the server terminates

hjk1030 commented May 10, 2024

View reviewed changes

ddkang reviewed May 10, 2024

View reviewed changes

Refactor cache clearing

195c8cf

* Use the existing inference topological order as delete order

ttt-77 reviewed May 15, 2024

View reviewed changes

hjk1030 added 3 commits May 15, 2024 23:41

Fix code style and service collection

ac4eeff

* Fix how the service to clear cache are collected * Reduce redundant table deletion * Comment Fix * Remove redundant sleep

Fix service collection

3a9e94e

* Use the correct graph to collect service

Fix the way getting edge attribute

9a0b072

* Fix

ttt-77 reviewed May 16, 2024

View reviewed changes

Merge test stages & Refactor cache clearing

0ece539

* Merge the two stage of testing using a loop * Refactor the cache clearing using the table graph

		@@ -1,13 +1,18 @@
		from multiprocessing import Process
		import os

		@@ -47,24 +51,35 @@ async def test_num_infer_calls(self):
		# no service calls before executing query


		asyncio_run(aidb_engine.clear_ml_cache(["lights01"]))

		for index, (query_type, aidb_query, exact_query) in enumerate(queries):

Add implementation for clearing cache table #177

Are you sure you want to change the base?

Add implementation for clearing cache table #177

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose 10000 a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!