bio-boris commented 3 years ago

Description of PR purpose/changes

In favor of https://github.com/kbase/execution_engine2/pull/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:

[x] My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/data-upload-project/development
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Any dependent changes have been merged and published in downstream modules
[x] 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)

MrCreosote commented 3 years ago

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

MrCreosote commented 3 years ago

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

lgtm-com[bot] commented 3 years ago

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

new alerts:

1 for Unused import

codecov[bot] commented 3 years ago

Codecov Report

Merging #406 (76fcb1a) into develop (1307bce) will increase coverage by 4.33%. The diff coverage is 92.58%.

:exclamation: Current head 76fcb1a differs from pull request most recent head f244544. Consider uploading reports for the commit f244544 to get more accurate results

@@             Coverage Diff             @@
##           develop     #406      +/-   ##
===========================================
+ Coverage    83.84%   88.17%   +4.33%     
===========================================
  Files           22       29       +7     
  Lines         1993     2799     +806     
===========================================
+ Hits          1671     2468     +797     
- Misses         322      331       +9

Impacted Files	Coverage Δ
lib/execution_engine2/utils/CondorTuples.py	`100.00% <ø> (ø)`
lib/execution_engine2/utils/SlackUtils.py	`35.71% <8.33%> (-7.53%)`	:arrow_down:
lib/execution_engine2/utils/KafkaUtils.py	`80.00% <66.66%> (+1.05%)`	:arrow_up:
lib/execution_engine2/authclient.py	`74.57% <74.57%> (ø)`
lib/execution_engine2/sdk/EE2Status.py	`86.13% <76.00%> (-2.22%)`	:arrow_down:
lib/execution_engine2/db/models/models.py	`92.55% <82.35%> (-1.17%)`	:arrow_down:
lib/execution_engine2/sdk/EE2Runjob.py	`91.22% <90.00%> (+6.41%)`	:arrow_up:
lib/execution_engine2/db/MongoUtil.py	`74.45% <92.85%> (+7.37%)`	:arrow_up:
lib/execution_engine2/sdk/EE2Logs.py	`96.51% <93.93%> (+5.40%)`	:arrow_up:
lib/execution_engine2/sdk/SDKMethodRunner.py	`84.42% <95.78%> (+5.09%)`	:arrow_up:
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3150622...f244544. Read the comment docs.

lgtm-com[bot] commented 3 years ago

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

new alerts:

1 for Unused import

bio-boris commented 3 years ago

Working on tests and coverage

lgtm-com[bot] commented 3 years ago

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

new alerts:

1 for Unused import

bio-boris commented 3 years ago

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 commented 3 years ago

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

MrCreosote commented 3 years ago

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

MrCreosote commented 3 years ago

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

kbase / execution_engine2

DATAUP-530 Refactor to bulk insert #406

Description of PR purpose/changes

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

Codecov Report