maxfischer2781 commented 2 years ago

This PR restructures the concurrency runners to be more robust. Major changes include:

Reduce public features of private types.
- [x] Remove paranoid thread concurrency protections.
[x] Switch runner core loops to asyncio from threading
- This should be more efficient due to less thread switching and waits.
[x] Move queueing-before-start mechanism to meta runner.
- This simplifies the actual runners.
- [x] Runners now use an async queue internally which avoids busy looping.
[x] Wrap main thread asyncio loop instead of creating a new loop
- I have no idea how that even worked before w.r.t #8.

Unittests are down from 15-20s to 2-3s now (which doesn't mean that cobald itself is that much faster.)

Note to reviewers:

The public interface of register_payload/run_payload has stayed intact but the parts used by cobald itself has changed considerably. Since the architecture changed (from threading to asyncio) it is probably not helpful to look just at the diff directly.

As a recap of the current architecture:

The user facing entry point is ServiceRunner, which has not changed other than some documentation.
The next step is MetaRunner, which now runs an asyncio loop instead of relying on ThreadRunner for concurrency.
- MetaRunner starts and monitors several BaseRunner instances concurrently.
The BaseRunner is the template for actual implementations.

lgtm-com[bot] commented 2 years ago

This pull request introduces 2 alerts and fixes 1 when merging 09846566afee7813f831a4d9f64aa1e283a2f7b7 into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

new alerts:

2 for Unused import

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging c3951e6f8eaab37c07ca168db6bf3347b987716f into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 582e39f55d211b44582c24271987cade9402a176 into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging d49247fb18d08ed3931bea5797daffaea9670567 into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

Hm, looks like codecov checking isn't working...

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 988cece888556af17965fd25a71f46ad044d447b into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

codecov[bot] commented 2 years ago

Codecov Report

Merging #109 (68b8369) into master (b33bb8e) will decrease coverage by 0.47%. The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage   93.45%   92.98%   -0.48%     
==========================================
  Files          35       34       -1     
  Lines        1146     1140       -6     
  Branches      164      166       +2     
==========================================
- Hits         1071     1060      -11     
- Misses         57       58       +1     
- Partials       18       22       +4

Impacted Files	Coverage Δ
src/cobald/daemon/runners/service.py	`91.17% <50.00%> (-0.91%)`	:arrow_down:
src/cobald/daemon/runners/asyncio_runner.py	`93.18% <91.89%> (-2.06%)`	:arrow_down:
src/cobald/daemon/runners/thread_runner.py	`93.75% <92.59%> (-4.37%)`	:arrow_down:
src/cobald/daemon/runners/meta_runner.py	`95.00% <92.85%> (-1.30%)`	:arrow_down:
src/cobald/daemon/runners/_compat.py	`93.93% <93.93%> (ø)`
src/cobald/daemon/runners/base_runner.py	`95.55% <96.00%> (-4.45%)`	:arrow_down:
src/cobald/daemon/runners/trio_runner.py	`100.00% <100.00%> (ø)`

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 b33bb8e...68b8369. Read the comment docs.

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 139d69af019b322a1a02782fa557b62195e627f2 into fdc3d2735cde075e5d0f3bf6511e31a1218a83fc - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 1bf99b85aabdd2baf8cc8dd6049734c11a765ba4 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 313343cf7df0700ed55f58224212c97d0954a492 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

Sorry, looks like I messed up some error handling during shutdown. Will fix...

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging de0cf3dcd69540230f8d4e8b33b00b1abea65b6e into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

Sorry for the hiccup @eileen-kuehn @mschnepf! The code now works as desired; I'm currently doing some cleanup and increasing the coverage.

Since I expect you'll be requesting changes anyway (even if it's just for clarification/documentation) please feel free to give this a first go already.

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging f776e0209d1aa23d33ad610a2f2ce6ae3afa33f6 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging e9e39eca8cc6e4e32bc74cf8f0734134a54b948b into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 2b74a7e2d9dc70865a2ccca0d46dd847fc6839ab into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

Alright, that's all the changes. Fittingly enough, I was not able to remove one of the concurrency issues the unittests have: When using a thread to run a runner in the background (e.g. test_meta_runner:threaded_run) sometimes the thread does not start at all. I have no idea why that is.

maxfischer2781 commented 2 years ago

👋 @eileen-kuehn @mschnepf

Let me know if you're tied up and I should request reviews from others.

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 2cb23fcb5d94915e46c63ae674ae17f7b0f63e44 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 825665bcf4bc27d9693d8fc945f1c73edec38c26 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 6d7c46a27999da94e3b8719a0ef3dc261189fa04 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

I think I got all comments covered now. Not sure on renaming MetaRunner, I think that's unfortunate but I really have no better idea.

Ready for the next round, reviewers? ;)

lgtm-com[bot] commented 2 years ago

This pull request fixes 1 alert when merging 68b83699e66f4e08d6a4046fd5f4f9513d8ade65 into b33bb8efd8c1c521349ab1366c9cf9e3ad78f5d2 - view on LGTM.com

fixed alerts:

1 for Except block handles 'BaseException'

maxfischer2781 commented 2 years ago

@mschnepf any objections to the new changes? Your last review was invalidated by them.

maxfischer2781 commented 2 years ago

@mschnepf I think you accidentally that other PR twice. Can you take another look at this one here?

MatterMiners / cobald

Robust Runner #109

Codecov Report