Mongo7 upgrade #140

Xiangs18 · 2024-12-17T22:50:16Z

No description provided.

codecov · 2024-12-17T22:54:31Z

Codecov Report

Attention: Patch coverage is 96.68874% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.81%. Comparing base (dd630ce) to head (0859dbe).

Files with missing lines	Patch %	Lines
lib/biokbase/catalog/db.py	96.68%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #140      +/-   ##
===========================================
+ Coverage    80.14%   80.81%   +0.67%     
===========================================
  Files            8        8              
  Lines         2695     2810     +115     
===========================================
+ Hits          2160     2271     +111     
- Misses         535      539       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bio-boris · 2025-01-23T17:11:37Z

test/mock_auth/server.py

+    sys.exit(1)
+
+endpoints = []
+catalog_mock_auth = ['auth_admin.json', 'auth_invalid.json', 'auth_missing.json', 'auth_non_admin.json']


It would be good to put a comment here explaining that the order is being preserved here in order to ensure the routes capture the requests in the correct order

Not sure if a comment was added or not, but additional info about "Mock server servers matches routes in a specific order, this is the current order required for tests to pass"

…v-mongo7_upgrade

bio-boris · 2025-02-05T22:51:11Z

lib/biokbase/catalog/db.py

+        self._mongo_client_initialized = False
+        self.mongo = None
+        self.db = None
+        self.index_created = defaultdict(int)


Should this be a bool?

I used 1 for True and 0 for False.

Yes, so why not use a defaultdict(bool) instead?

Xiangs18 · 2025-02-06T21:29:36Z

This PR does the following:

Removed all submodules (jars, kbapi_common, nms).
Added the NMS Docker service for local testing.
Fixed a bug in mock_auth/servicer.py.
Updated the catalog Docker build script.
Implemented lazy loading for MongoDB.
Cleaned up files in the repository.

bio-boris

The pr description is empty, and would benefit from a copy of the changelog/release notes . The version and release notes should be updated as well

bio-boris · 2025-02-06T21:35:28Z

Makefile

@@ -27,11 +27,6 @@ LIB_DIR = lib

 PATH := kb_sdk/bin:$(PATH)

-default: init
-
-init:


Does this init section need to be removed or can we keep it in?

Or was this deprecated in favor of docker compose, and was only needed for tests?

All submodules have been removed, so there is no need to pull them. If you want to keep init section, what should be included in the init?

bio-boris · 2025-02-06T21:35:50Z

Makefile

@@ -102,8 +97,8 @@ setup-tests:
 	mkdir -p $(TESTLIB)/biokbase
 	mkdir -p $(TESTDIR)/nms
 	rsync -av lib/biokbase/* $(TESTLIB)/biokbase/. --exclude *.bak-*
-	rsync -av kbapi_common/lib/biokbase/* $(TESTLIB)/biokbase/.


If we don't need this anymore, can we delete it instead of commenting it out?

bio-boris · 2025-02-06T21:37:45Z

.github/workflows/test.yml

    services:
      mongo:
        image: mongo:${{matrix.mongo-version}}
        ports:
-            - 27017:27017
+            - 27018:27017


Can we keep it at 27017?

port 27017 is already being used by the NMS Mongo

I would change the NMS port in that case. If one or the other is going to have a non standard port it should be the supporting system, not the one we're working on

Why do we need 2 seperate mongos?

One presumably starts up with the docker compose to support NMS, the other starts up as part of the catalog tests. That seems good to me, the catalog shouldn't know or care about any NMS dependencies

bio-boris · 2025-02-06T21:41:21Z

.github/workflows/test.yml


      - name: Setup Python
        uses: actions/setup-python@v5
        with:
          python-version: ${{matrix.python-version}}

+      - name: Install Docker Compose


This is not necessary. You can use docker compose as it is included in docker nowadays

This step is necessary.

https://github.com/kbase/catalog/actions/runs/13210034758/job/36881655854?pr=140#step:6:43

Try docker compose without the hyphen

bio-boris · 2025-02-06T21:44:11Z

test/mock_auth/server.py

+    sys.exit(1)
+
+endpoints = []
+catalog_mock_auth = ['auth_admin.json', 'auth_invalid.json', 'auth_missing.json', 'auth_non_admin.json']


Not sure if a comment was added or not, but additional info about "Mock server servers matches routes in a specific order, this is the current order required for tests to pass"

bio-boris · 2025-02-06T21:44:57Z

test/test.cfg.example

-# host where mongo lives, e.g. localhost:27017, for tests there should be not authentication required
-mongodb-host = localhost:27017
+# host where mongo lives, e.g. localhost:27018, for tests there should be not authentication required
+mongodb-host = localhost:27018


Can we keep this at 27017?

port 27017 is already being used by the NMS Mongo

bio-boris · 2025-02-06T21:50:12Z

lib/biokbase/catalog/db.py

+                unique=True, sparse=False)
+
+        elif collection_name == MongoCatalogDBI._LOCAL_FUNCTIONS:
+        # Make sure we have a unique index on module_name_lc and git_commit_hash


It is possible to refactor this giant elif block using a dictionary to map collection names to their respective index creation functions. Here is an example of how you can refactor it:

def _create_indexes(self, collection_name): """Create indexes for the given collection lazily.""" collection = self.db[collection_name] index_creation_map = { MongoCatalogDBI._MODULE_VERSIONS: [ ('module_name_lc', False), ('git_commit_hash', False), [('module_name_lc', ASCENDING), ('git_commit_hash', ASCENDING), True] ], MongoCatalogDBI._LOCAL_FUNCTIONS: [ 'function_id', [('module_name_lc', ASCENDING), ('function_id', ASCENDING), ('git_commit_hash', ASCENDING), True], 'module_name_lc', 'git_commit_hash' ], # Add other collections and their indexes similarly } indexes = index_creation_map.get(collection_name, []) for index in indexes: if isinstance(index, tuple): collection.create_index(index[0], sparse=index[1]) elif isinstance(index, list): collection.create_index(index, unique=index[-1]) else: collection.create_index(index)

This approach uses a dictionary to map collection names to a list of indexes, which reduces the need for multiple elif statements. Adjust the index_creation_map dictionary as needed to cover all your collections and their respective indexes.

You could potentially use an index spec to simplify as well: https://github.com/kbase/cdm-task-service/blob/main/cdmtaskservice/mongo.py#L70

bio-boris · 2025-02-06T21:51:30Z

lib/biokbase/catalog/db.py

+        return self._get_collection(MongoCatalogDBI._SECURE_CONFIG_PARAMS)
+
+    def _create_indexes(self, collection_name):
+        """Create indexes for the given collection lazily."""


It might be better to move out these into smaller functions into a file or class like db_indexes.py or something as this is an extremely long function. If possible, function's shouldn't be longer than the entire height of your screen.

bio-boris · 2025-02-06T21:52:38Z

lib/biokbase/catalog/db.py

@@ -1274,6 +1360,12 @@ def get_secure_config_params(self, module_name):
    # another server is already starting an update, we can skip or abort
    def check_db_schema(self):

+        if self._db_schema_checked:
+            return


This is missing coverage

bio-boris · 2025-02-06T21:55:15Z

lib/biokbase/catalog/db.py

@@ -1455,20 +1551,23 @@ def prepare_version_doc_for_db_2_to_3_update(self, version, module):
    def update_db_3_to_4(self):

        # make sure we don't have any indecies on the collections
-        self.volume_mounts.drop_indexes()
-        self.client_groups.drop_indexes()
+        volume_mounts_collection = self.db[MongoCatalogDBI._VOLUME_MOUNTS]


I think this entire function can be deleted, as we have already upgraded the indexes?

AFAIK we're the only ones using the catalog, all these db version changes happened years ago, and so we can just put a note in the release notes that db version upgrades are no longer supported and delete all the upgrade methods

bio-boris · 2025-02-06T21:58:23Z

test/docker-compose_nms.yml

@@ -0,0 +1,47 @@
+version: '3.4'
+


Can we re-name this file as "docker-compose?"

If this is a compose file for the nms rather than the catalog, calling it as such seems appropriate to me

It is a docker compose for testing purposes. By naming it "docker-compose" it allows you to start it up without specifying the path to a file. If we had multiple docker composes, I think it makes sense to use a non standard name, but since we only have one docker-compose file, we should use the standard name.

It's just for starting the NMS service for CI/local testing to run unit tests. It makes more sense to keep its name as is.

I'm fine with -f <file> in this case but I don't feel that strongly about it

bio-boris · 2025-02-06T22:01:58Z

This PR does the following:

1. Removed all submodules (jars, kbapi_common, nms). (Why though?)

2. Added the NMS Docker service for local testing. 

3. Fixed a bug in mock_auth/servicer.py. (Could you add more details to this?)

4. Updated the catalog Docker build script. (More details please)

5. Implemented lazy loading for MongoDB. (More details please)

6. Cleaned up files in the repository. (I don't know what this means)

This also upgrades pymongo

I think these can be added to the README.md

MrCreosote · 2025-02-07T21:28:59Z

.github/workflows/test.yml

@@ -77,6 +81,10 @@ jobs:
          sed -i "s#^nms-admin-token.*#nms-admin-token=$KBASE_CI_TOKEN#" test/test.cfg
          sed -i "s#^method-spec-admin-users.*#method-spec-admin-users=$ADMIN_USER#" test/test.cfg

+          # setup env variables in docker-compose_nms.yml file
+          echo "ADMIN_USER=$ADMIN_USER"
+          echo "ADMIN_USER=$ADMIN_USER" >> $GITHUB_ENV


Could you add a comment about why this env var is set up and where it's used? That should help the next person who has to work on this service understand what's going on

MrCreosote · 2025-02-07T21:31:15Z

Dockerfile

 COPY requirements.txt requirements.txt
-RUN source activate root && \
-    pip install -r requirements.txt
+RUN pip install -r requirements.txt


We should convert to pipenv / poetry / uv, but that's a later task

lib/biokbase/log.py

MrCreosote · 2025-02-07T21:41:44Z

test/run_tests.sh

 NMS_PID=$!

 echo 'Starting Mock Auth API...'
-docker run -d --rm -v ${PWD}/mock_auth:/config -p 7777:5000 --name mock-auth mockservices/mock_json_service
+docker run -d --rm -v ${PWD}/mock_auth:/config -v ${PWD}/mock_auth/server.py:/server/server.py -p 7777:5000 --name mock-auth mockservices/mock_json_service


This isn't something we need to deal with now, but there are plenty of 3rd party service mocking options out there, I'm not sure why we rolled our own. Also auth2 has test mode which we could use

MrCreosote · 2025-02-07T21:42:55Z

test/run_tests.sh


 echo 'Waiting for NMS to start...'
 sleep 25
-curl -d '{"id":"1","params":[],"method":"NarrativeMethodStore.ver","version":"1.1"}' http://localhost:7125
+curl -d '{"id":"1","params":[],"method":"NarrativeMethodStore.ver","version":"1.1"}' http://localhost:7125/rpc


Wow these tests must not've passed for a long time...

MrCreosote · 2025-02-07T21:46:57Z

test/mock_auth/server.py

This file needs some explanation at the top. Why are we replacing the file in the mock server container? Whoever comes along next to work on the catalog will need to understand the reasoning behind this

MrCreosote · 2025-02-07T21:47:23Z

test/mock_auth/server.py

What changes have been made in this file compared to the original in the mock server container?

MrCreosote · 2025-02-07T21:52:11Z

lib/biokbase/catalog/db.py

+        self.mongo_psswd = mongo_psswd
+        self.mongo_authMechanism = mongo_authMechanism
+
+        # MongoDB client and database initialization deferred


Can you add an explanation as to why this is necessary?

MrCreosote · 2025-02-07T21:53:12Z

lib/biokbase/catalog/db.py

+        self.mongo_psswd = mongo_psswd
+        self.mongo_authMechanism = mongo_authMechanism
+
+        # MongoDB client and database initialization deferred


How lazy is lazy? Is it not until a request is made, and therefore the service can be running in a guaranteed to fail state? If so, it seems like this class should at least test connectivity and auth at init time

IIUC the db class instance is created pre fork, and then the fork happens and it's no longer working. If that's the case, and if it's possible, what I would do is

in the init, check connectivity, check db schema, create indexes, etc., and close the client.

On the first call, create a mongo client knowing that everything is set up correctly, and set up the instance variables for the collections. Then you don't need all the property functions

Oh, also you probably need a thread lock around the client creation step, or multiple calls could result in multiple client creations

MrCreosote · 2025-02-07T21:54:06Z

lib/biokbase/catalog/db.py

+        """Initialize MongoDB client and collections lazily."""
+        if not self._mongo_client_initialized:
+            try:
+                if self.mongo_user and self.mongo_psswd:


Can you add a note here that this case is only tested manually?

I assume it has been tested manually, yeah?

mongo7 upgrade

6ec3d63

Xiangs18 added 16 commits January 6, 2025 10:10

upgrade pymongo to 4.7.2

26b7d9e

update python to 3.9.19

c5f1294

update Makefile and run_tests.sh scripts

1e57094

update submodules

47ff558

fix gradle not found

4e1f8bf

start nms service with docker-compose

4e0c2dc

update nms submodule to the latest

8095c7f

install docker-compose in GHA

6f22c74

switch catalog mongo7 port to 27018

dff0b20

fix mongo port

76bff46

fix mongo port mapping

0b7cd2a

set admin_users

66f2485

check admin users

85c65af

shut down NMS service properly

9fda21f

fix nms curl

91c72f7

update mock_auth service

dd1d2da

bio-boris reviewed Jan 23, 2025

View reviewed changes

Xiangs18 and others added 11 commits January 27, 2025 13:00

update the commit

2e84037

Update test.cfg.example

727753c

Update test.cfg.example

77d161b

Update core_registration_test.py

bca78a9

update message

178a63c

Merge branch 'dev-mongo7_upgrade' of github.com:kbase/catalog into de…

a3b1038

…v-mongo7_upgrade

test method_spec_admin_users

5379082

test simple_good_repo old commit

631b52f

test register hack

b412672

remove jars nms submodules

80be977

update Dockerfile

7ead4f7

Xiangs18 added 17 commits January 28, 2025 17:14

remove kbapi_common submodule

01f1539

fix deprecated mongo auth

35a18c4

remove all submodules in catalog

c228438

create MongoClient through passing in params

9153235

add a comment for mock_auth server.py

2344d64

simplify code

f147120

adjust params in MongoClient

7a5b354

add debug log messages in list_user_favorites func

3e3673f

try reinitiate mongoclient

093510e

update __init__ in db.py file

ec3064b

fix typo

4aabfb9

add check_db_schema

fe87438

add boolean flag for schema check

5af0ccd

collection direct call

93e0033

remove debugging message

37f083d

display catalog version

d0c2522

fix failed unit test in startup_test.py

6b5fcde

bio-boris reviewed Feb 5, 2025

View reviewed changes

Xiangs18 added 2 commits February 5, 2025 15:00

trigger lazy load collection

0143000

clean up files

d04e2eb

Xiangs18 changed the title ~~[WIP] mongo7 upgrade~~ Mongo7 upgrade Feb 6, 2025

Xiangs18 requested review from MrCreosote and bio-boris February 6, 2025 21:31

bio-boris requested changes Feb 6, 2025

View reviewed changes

bio-boris reviewed Feb 6, 2025

View reviewed changes

MrCreosote reviewed Feb 7, 2025

View reviewed changes

Xiangs18 added 2 commits February 7, 2025 15:59

remove comments from Makefile

59c653e

validate Install Docker Compose step in ci

0859dbe

Mongo7 upgrade #140

Are you sure you want to change the base?

Mongo7 upgrade #140

Conversation

Xiangs18 commented Dec 17, 2024

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiangs18 commented Feb 6, 2025

bio-boris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiangs18 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiangs18 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bio-boris commented Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 •

edited

Loading

Xiangs18 Feb 7, 2025 •

edited

Loading

MrCreosote Feb 7, 2025 •

edited

Loading

Xiangs18 Feb 7, 2025 •

edited

Loading

bio-boris commented Feb 6, 2025 •

edited

Loading

MrCreosote Feb 7, 2025 •

edited

Loading

MrCreosote Feb 7, 2025 •

edited

Loading