Check dataset connection before migration job (and other apps) #792

severo · 2023-02-08T17:14:07Z

Before starting the services (api, admin) and workers, we ensure the database is accessible and the assets directory (if needed) exists.

In the case of the migration job: if the database cannot be accessed, we skip the migration, to avoid blocking Helm

Fixes #763. Replaces #767. Depends on #791.

codecov-commenter · 2023-02-08T17:15:55Z

Codecov Report

Base: 90.72% // Head: 90.66% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (aadaa18) compared to base (f0d048e).
Patch coverage: 83.78% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
- Coverage   90.72%   90.66%   -0.06%     
==========================================
  Files         105      105              
  Lines        5259     5292      +33     
==========================================
+ Hits         4771     4798      +27     
- Misses        488      494       +6

Flag	Coverage Δ
jobs_mongodb_migration	`80.29% <100.00%> (+0.04%)`	⬆️
libs_libcommon	`92.82% <94.73%> (+0.02%)`	⬆️
services_admin	`86.84% <70.00%> (-0.29%)`	⬇️
services_api	`89.27% <71.42%> (-0.23%)`	⬇️
workers_datasets_based	`92.82% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
libs/libcommon/src/libcommon/storage.py	`95.45% <66.66%> (-4.55%)`	⬇️
services/admin/src/admin/app.py	`88.63% <70.00%> (-5.81%)`	⬇️
services/api/src/api/app.py	`90.47% <71.42%> (-3.97%)`	⬇️
jobs/mongodb_migration/tests/test_resources.py	`100.00% <100.00%> (ø)`
libs/libcommon/src/libcommon/resources.py	`95.55% <100.00%> (+1.11%)`	⬆️
libs/libcommon/tests/test_resources.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

HuggingFaceDocBuilder · 2023-02-08T17:17:13Z

The documentation is not available anymore as the PR was closed or merged.

severo · 2023-02-10T16:33:52Z

@rtrompier this PR should fix #763.

AndreaFrancis · 2023-02-10T16:43:34Z

jobs/mongodb_migration/src/mongodb_migration/main.py

    ):
+        if cache_resource.check() is False:
+            logging.info(


Not sure if it should be an info, maybe Warning since it will avoid running the migrations

albertvillanova

Some nits below...

As this function returns a boolean, maybe you could try to find a name using the third-person singular, like the pathlib does for path.exists()...

jobs/mongodb_migration/src/mongodb_migration/main.py

jobs/mongodb_migration/tests/test_resources.py

libs/libcommon/tests/test_resources.py

services/admin/src/admin/app.py

services/api/src/api/app.py

albertvillanova · 2023-02-10T16:45:14Z

workers/datasets_based/src/datasets_based/main.py

+        if libraries_resource.check() is False:
+            raise RuntimeError("The datasets and numba libraries could not be configured. Exiting.")
+        if cache_resource.check() is False:
+            raise RuntimeError("The connection to the cache database could not be established. Exiting.")
+        if queue_resource.check() is False:
+            raise RuntimeError("The connection to the queue database could not be established. Exiting.")
+


The same here...

albertvillanova · 2023-02-10T16:45:36Z

workers/datasets_based/src/datasets_based/resources.py

@@ -51,6 +51,9 @@ def allocate(self):
            storage_paths.add(self.numba_path)
        self.storage_paths = storage_paths

+    def check(self) -> bool:
+        return True


What is the point of this function?

I had put check() in the base class with NotImplementedError, so I had to implement it. But it was poor design, I moved it to MongoResource, now it's more logical.

workers/datasets_based/tests/test_resources.py

@AndreaFrancis

thanks @AndreaFrancis

Co-authored-by: Albert Villanova del Moral <[email protected]>

@albertvillanova

thanks @albertvillanova

severo · 2023-02-10T17:04:01Z

As this function returns a boolean, maybe you could try to find a name using the third-person singular, like the pathlib does for path.exists()...

good advice, I'll try to remember it for the next time too

albertvillanova

Thanks!

severo · 2023-02-10T17:20:36Z

Thanks for the reviews!!!

severo mentioned this pull request Feb 8, 2023

Check dataset before migration job #767

Closed

severo mentioned this pull request Feb 9, 2023

Move workers/datasets_based to services/worker #800

Merged

severo force-pushed the use-classmethod branch from d280c69 to b1b0329 Compare February 10, 2023 15:57

severo force-pushed the check-dataset-connection-before-migration-job branch from c284964 to a334a52 Compare February 10, 2023 16:22

severo force-pushed the use-classmethod branch from b1b0329 to 66b475c Compare February 10, 2023 16:24

severo force-pushed the check-dataset-connection-before-migration-job branch from a334a52 to 92948e6 Compare February 10, 2023 16:25

severo requested review from rtrompier, albertvillanova, AndreaFrancis and lhoestq February 10, 2023 16:27

severo marked this pull request as ready for review February 10, 2023 16:27

Base automatically changed from use-classmethod to main February 10, 2023 16:29

severo force-pushed the check-dataset-connection-before-migration-job branch from 92948e6 to 4c1d030 Compare February 10, 2023 16:30

severo added 7 commits February 10, 2023 16:31

feat: 🎸 use primitive parameters, add release, add tests

1facd66

style: 💄 fix style

ccdc462

feat: 🎸 use primitive parameters, add release, add tests

835aefc

style: 💄 fix style

303c982

feat: 🎸 add a check() method to test a mongo connection

165f0b3

feat: 🎸 add a check() method in Resource

57dab13

feat: 🎸 check if the resources are available at start up

ce2b651

severo force-pushed the check-dataset-connection-before-migration-job branch from 4c1d030 to ce2b651 Compare February 10, 2023 16:31

severo changed the title ~~Check dataset connection before migration job~~ Check dataset connection before migration job (and other apps) Feb 10, 2023

AndreaFrancis reviewed Feb 10, 2023

View reviewed changes

albertvillanova reviewed Feb 10, 2023

View reviewed changes

severo and others added 3 commits February 10, 2023 17:47

log a warning when the migration cannot access database

d61cd29

thanks @AndreaFrancis

Update libs/libcommon/tests/test_resources.py

1fcec1e

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update jobs/mongodb_migration/src/mongodb_migration/main.py

a81e6c0

Co-authored-by: Albert Villanova del Moral <[email protected]>

severo and others added 12 commits February 10, 2023 17:48

Update jobs/mongodb_migration/tests/test_resources.py

e3886ce

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update libs/libcommon/tests/test_resources.py

202d637

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update services/api/src/api/app.py

4e7b028

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update workers/datasets_based/tests/test_resources.py

f98ace3

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update workers/datasets_based/tests/test_resources.py

876516a

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update services/admin/src/admin/app.py

6adc5d7

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update services/admin/src/admin/app.py

07e45f4

Co-authored-by: Albert Villanova del Moral <[email protected]>

Update services/api/src/api/app.py

7323f17

Co-authored-by: Albert Villanova del Moral <[email protected]>

refactor: 💡 simpler

bdc09c6

refactor: 💡 use third-person verb for bool + only for database

ad18503

thanks @albertvillanova

fix: 🐛 fix method name

ba905d7

style: 💄 remove unused variable

aadaa18

albertvillanova approved these changes Feb 10, 2023

View reviewed changes

AndreaFrancis approved these changes Feb 10, 2023

View reviewed changes

severo merged commit 6666942 into main Feb 10, 2023

severo deleted the check-dataset-connection-before-migration-job branch February 10, 2023 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check dataset connection before migration job (and other apps) #792

Check dataset connection before migration job (and other apps) #792

severo commented Feb 8, 2023 •

edited

Loading

codecov-commenter commented Feb 8, 2023 •

edited

Loading

HuggingFaceDocBuilder commented Feb 8, 2023 •

edited

Loading

severo commented Feb 10, 2023

AndreaFrancis Feb 10, 2023

severo Feb 10, 2023

severo Feb 10, 2023

albertvillanova left a comment

albertvillanova Feb 10, 2023

severo Feb 10, 2023

albertvillanova Feb 10, 2023

severo Feb 10, 2023

severo commented Feb 10, 2023

albertvillanova left a comment

severo commented Feb 10, 2023

Check dataset connection before migration job (and other apps) #792

Check dataset connection before migration job (and other apps) #792

Conversation

severo commented Feb 8, 2023 • edited Loading

codecov-commenter commented Feb 8, 2023 • edited Loading

Codecov Report

HuggingFaceDocBuilder commented Feb 8, 2023 • edited Loading

severo commented Feb 10, 2023

AndreaFrancis Feb 10, 2023

Choose a reason for hiding this comment

severo Feb 10, 2023

Choose a reason for hiding this comment

severo Feb 10, 2023

Choose a reason for hiding this comment

albertvillanova left a comment

Choose a reason for hiding this comment

albertvillanova Feb 10, 2023

Choose a reason for hiding this comment

severo Feb 10, 2023

Choose a reason for hiding this comment

albertvillanova Feb 10, 2023

Choose a reason for hiding this comment

severo Feb 10, 2023

Choose a reason for hiding this comment

severo commented Feb 10, 2023

albertvillanova left a comment

Choose a reason for hiding this comment

severo commented Feb 10, 2023

severo commented Feb 8, 2023 •

edited

Loading

codecov-commenter commented Feb 8, 2023 •

edited

Loading

HuggingFaceDocBuilder commented Feb 8, 2023 •

edited

Loading