DATAUP-530 Refactor to bulk insert by bio-boris · Pull Request #406 · kbase/execution_engine2

bio-boris · 2021-07-16T17:12:32Z

Description of PR purpose/changes

In favor of DATAUP-530 Multiple submissions #403
Use a mongoengine bulk create (with no limit on number of docs that can be written)
Use a pymongo bulk update job status
Change kafka creation message order
Batch submits via the bulk submit.

Testing Instructions

Deployed on CI

Dev Checklist:

My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/data-upload-project/development
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have run Black and Flake8 on changed Python Code manually or with git precommit (and the Github Actions build passes)

Updating Version and Release Notes (if applicable)

Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

lib/execution_engine2/db/MongoUtil.py

lib/execution_engine2/sdk/EE2Runjob.py

MrCreosote

Mostly looks pretty reasonable. I have a few questions re error handling and some internal api suggestions

lib/execution_engine2/db/MongoUtil.py

lib/execution_engine2/sdk/EE2Runjob.py

MrCreosote · 2021-07-19T20:54:59Z

Oh, there's a couple of checklist things that aren't checked or marked as N/A, missed that

lib/execution_engine2/db/MongoUtil.py

lib/execution_engine2/sdk/EE2Runjob.py

MrCreosote

Couple questions on the new stuff

MrCreosote

couple questions on the new stuff

MrCreosote · 2021-07-21T20:59:21Z

Well that's weird, not quite sure what happened there

MrCreosote

Taking a look at the newer changes

test/tests_for_db/ee2_MongoUtil_test.py

test/tests_for_sdkmr/EE2Runjob_test.py

bio-boris · 2021-08-02T19:58:59Z

Working on tests and coverage

lgtm-com · 2021-08-02T20:24:14Z

This pull request introduces 1 alert when merging 9be3578 into 3150622 - view on LGTM.com

new alerts:

1 for Unused import

bio-boris · 2021-08-02T20:24:48Z

test/utils_shared/producer.py

-            )
        except Exception as e:
-            logging.info(
+            logging.error(


Removed extra info from logs

bio-boris · 2021-08-02T20:25:00Z

test/tests_for_sdkmr/ee2_kafka_test.py

    def test_status_change(self):

-        with self.assertRaisesRegexp(
+        with self.assertRaisesRegex(


fixing warning

bio-boris · 2021-08-02T20:25:05Z

test/tests_for_sdkmr/ee2_SDKMethodRunner_test_EE2Runjob_test.py

            "hex string"
        )
-        with self.assertRaisesRegexp(RetryFailureException, errmsg):
+        with self.assertRaisesRegex(RetryFailureException, errmsg):


fixing warning

bio-boris · 2021-08-02T20:25:17Z

test/tests_for_sdkmr/ee2_SDKMethodRunner_test.py

-from lib.execution_engine2.exceptions import InvalidStatusTransitionException
-from lib.execution_engine2.sdk.SDKMethodRunner import SDKMethodRunner
-from lib.execution_engine2.utils.CondorTuples import SubmissionInfo
+from execution_engine2.db.models.models import Job, Status, TerminatedCode


fixing imports

MrCreosote

code seems fine but the tests aren't passing, so I assume there's more work there to do and haven't looked at them yet

lib/execution_engine2/sdk/EE2Runjob.py

lib/execution_engine2/db/MongoUtil.py

test/tests_for_db/ee2_MongoUtil_test.py

bio-boris · 2021-08-03T03:41:35Z

Tests are done, not sure why that one failed all of a sudden on github actions. It's been failing locally on my machine for months but not on github actions. It was a bad test because the underlying functionality is actually wrong and the test is flaky depending on where you run it

MrCreosote · 2021-08-04T19:55:56Z

Interesting, the tests passed consistently on my machine when I was doing ee2 work.

MrCreosote · 2021-08-04T20:26:58Z

Note to self: New behaviors are

Batch jobs are saved via SDKMR.save_jobs vs save_job (calling MongoUtil.insert_jobs)
Batch jobs are set to updated by MongoUtil.update_jobs_to_queued
Any batch jobs canceled prior to the update in 2) have SDKMR.cancel_job called on them
Parent job is updated with child jobs via SDKMR.add_child_jobs

MrCreosote · 2021-08-04T20:50:20Z

Further note to self: there are API to DB integration tests that exercise run_batch which pass without changes, which is reassuring

MrCreosote

Looks good, just a few minor changes and some unit tests that I think should be pretty easy

lib/execution_engine2/db/MongoUtil.py

MrCreosote · 2021-08-04T20:32:15Z

lib/execution_engine2/db/MongoUtil.py

        yield self.me_connection

+    def insert_jobs(self, jobs_to_insert: List[Job]) -> List[ObjectId]:
+        """


There aren't any unit tests for this function

Ok, wrote a basic test

It tests that the method returns job IDs, but not that the jobs are actually correctly inserted to the DB. If this were me writing the tests I'd retrieve the jobs either via the MongoUtil API or directly from mongo and ensure the data was what I expected

Alright, added that, but not quite sure if that's what you had in mind

it is, except the problem is that equality on Job objects only tests the _id, not any other fields. So the insert method could completely trash the incoming Job object and save it and the test will still pass. I'd update to use the assert_job_equal method from EE2RunJob_test.py (maybe that should be moved to a utility class at some point)

Alright well how about using

for i, retrieved_job in enumerate(retrieved_jobs): assert jobs_to_insert[i].to_json() == retrieved_job.to_json() assert jobs_to_insert[i].to_mongo() == retrieved_job.to_mongo() assert jobs_to_insert[i].to_dbref() == retrieved_job.to_dbref()

Seems reasonable

Bah, realized that the modify command isn't updating the "updated" timestamp

Added this test in a new PR #416

lib/execution_engine2/db/MongoUtil.py

lib/execution_engine2/sdk/SDKMethodRunner.py

lib/execution_engine2/sdk/EE2Runjob.py

test/tests_for_sdkmr/EE2Runjob_test.py

MrCreosote

LGTM

Refactor to bulk insert

e9ac3c2

bio-boris commented Jul 16, 2021

View reviewed changes

lib/execution_engine2/db/MongoUtil.py Outdated Show resolved Hide resolved

bio-boris added 4 commits July 16, 2021 15:33

Refactor to bulk update

b3884a6

Timings

a8cc2ae

Timings

606d292

Timings

18b0109

bio-boris commented Jul 16, 2021

View reviewed changes

lib/execution_engine2/sdk/EE2Runjob.py Outdated Show resolved Hide resolved

Timings

9dc9fdf

bio-boris requested a review from MrCreosote July 16, 2021 22:17

MrCreosote requested changes Jul 19, 2021

View reviewed changes

bio-boris added 4 commits July 19, 2021 22:13

PR review

6404b86

Try to prevent bad updates for cancelled jobs

0b65043

Try to prevent bad updates for cancelled jobs

400c1ab

Not so trivial after all

f16943a

bio-boris requested a review from MrCreosote July 20, 2021 19:50

MrCreosote reviewed Jul 21, 2021

View reviewed changes

lib/execution_engine2/db/MongoUtil.py Outdated Show resolved Hide resolved

lib/execution_engine2/db/MongoUtil.py Outdated Show resolved Hide resolved

lib/execution_engine2/sdk/EE2Runjob.py Outdated Show resolved Hide resolved

MrCreosote requested changes Jul 21, 2021

View reviewed changes

bio-boris added 4 commits July 21, 2021 22:01

Pr feedback

ad74923

Fix up tests

dd8cfcf

Merge branch 'develop' into multiple_inserts

36cbb6f

Fix up assert

1d977a6

bio-boris requested a review from MrCreosote July 22, 2021 22:34

MrCreosote requested changes Jul 22, 2021

View reviewed changes

test/tests_for_db/ee2_MongoUtil_test.py Outdated Show resolved Hide resolved

test/tests_for_db/ee2_MongoUtil_test.py Outdated Show resolved Hide resolved

test/tests_for_db/ee2_MongoUtil_test.py Outdated Show resolved Hide resolved

test/tests_for_sdkmr/EE2Runjob_test.py Show resolved Hide resolved

bio-boris added 4 commits July 22, 2021 18:16

Fix missing test

cac5241

FIx tests

c3e08b3

Fix comment

a7256a4

Merge branch 'develop' into multiple_inserts

f55ab10

bio-boris requested review from MrCreosote and removed request for MrCreosote August 2, 2021 19:58

Increase coverage

9be3578

bio-boris commented Aug 2, 2021

View reviewed changes

test/utils_shared/producer.py

)

except Exception as e:

logging.info(

logging.error(

Copy link

Collaborator Author

bio-boris Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed extra info from logs

bio-boris commented Aug 2, 2021

View reviewed changes

MrCreosote requested changes Aug 2, 2021

View reviewed changes

lib/execution_engine2/sdk/EE2Runjob.py Show resolved Hide resolved

lib/execution_engine2/db/MongoUtil.py Outdated Show resolved Hide resolved

test/tests_for_db/ee2_MongoUtil_test.py Outdated Show resolved Hide resolved

bio-boris added 3 commits August 2, 2021 22:29

Removed unused, fixed tests

0183f98

Removed unused, fixed tests

af03d77

Removed unused, fixed tests

76fcb1a

bio-boris requested a review from MrCreosote August 3, 2021 03:41

MrCreosote requested changes Aug 4, 2021

View reviewed changes

bio-boris added 4 commits August 5, 2021 01:07

Add tests

3d517d9

Add tests

8e19ccf

pr feedback

2822e77

fix indent

ac4b6af

bio-boris requested a review from MrCreosote August 6, 2021 17:37

bio-boris added this to the Post-GSP-Release milestone Aug 6, 2021

Check job contents

f244544

MrCreosote approved these changes Aug 6, 2021

View reviewed changes

bio-boris merged commit 8ee6c0b into develop Aug 6, 2021

bio-boris deleted the multiple_inserts branch August 6, 2021 21:49

bio-boris mentioned this pull request Aug 6, 2021

Fix modify and save test, add "updated" to modify #416

Open

Conversation

bio-boris commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR purpose/changes

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

Uh oh!

Uh oh!

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrCreosote commented Jul 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

MrCreosote commented Jul 21, 2021

Uh oh!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bio-boris commented Aug 2, 2021

Uh oh!

lgtm-com bot commented Aug 2, 2021

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!

MrCreosote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bio-boris commented Aug 3, 2021

Uh oh!

MrCreosote commented Aug 4, 2021

Uh oh!

MrCreosote commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrCreosote commented Aug 4, 2021

Uh oh!

MrCreosote left a comment

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!

bio-boris commented Jul 16, 2021 •

edited

Loading

MrCreosote commented Aug 4, 2021 •

edited

Loading

bio-boris Aug 6, 2021 •

edited

Loading