YangCatalog / backend

YANG Catalog's REST API and internal module processing pipeline
https://yangcatalog.org
Apache License 2.0
2 stars 11 forks source link

chore: remove sender/receiver #767

Closed richardzilincikPantheon closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #767 (c85d9ef) into develop (a513302) will increase coverage by 1.10%. The diff coverage is 92.83%.

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

@@             Coverage Diff             @@
##           develop     #767      +/-   ##
===========================================
+ Coverage    76.60%   77.71%   +1.10%     
===========================================
  Files          109      108       -1     
  Lines        11603    11288     -315     
===========================================
- Hits          8889     8772     -117     
+ Misses        2714     2516     -198     
Impacted Files Coverage Δ
api/views/health_check.py 20.35% <ø> (+0.75%) :arrow_up:
api/job_runner.py 89.36% <89.36%> (ø)
api/views/user_specific_module_maintenance.py 89.69% <92.30%> (-0.46%) :arrow_down:
api/my_flask.py 62.69% <100.00%> (-0.39%) :arrow_down:
api/views/admin.py 97.15% <100.00%> (+0.22%) :arrow_up:
api/views/yc_jobs.py 52.05% <100.00%> (-1.28%) :arrow_down:
tests/test_api_admin.py 99.39% <100.00%> (+<0.01%) :arrow_up:
tests/test_api_contribute.py 99.03% <100.00%> (+0.01%) :arrow_up:
tests/test_api_internal.py 98.71% <100.00%> (+0.01%) :arrow_up:
tests/test_job_runner.py 91.70% <100.00%> (ø)
... and 2 more

... and 1 file with indirect coverage changes

richardzilincikPantheon commented 1 year ago

And maybe it's also worth it to set the max workers of the Pool to 1, so we don't get inconsistent results from different scripts running simultaneously, or if the same script is being executed twice at the same time?

Yeah that's true. It will still be (and has been) possible to run one of the scripts while they're already running in a cronjob. That should be fixed in a separate PR.

richardzilincikPantheon commented 1 year ago

Don't you think that writing job statuses to separate files would be better, so we can see if there were some jobs running while the container was rebuilt for example?

There's already logging in some of methods of the job runner. Would it be fine to just add logging to the places it's missing?

richardzilincikPantheon commented 1 year ago

I found out that this won't work correctly with creating a pool in the server process. I'll implement running the processes differently.

bskqd commented 1 year ago

Don't you think that writing job statuses to separate files would be better, so we can see if there were some jobs running while the container was rebuilt for example?

There's already logging in some of methods of the job runner. Would it be fine to just add logging to the places it's missing?

I think yes, I believe it would be better to add logging to the missing places

richardzilincikPantheon commented 1 year ago

So after more investigation, it looks like a generally bad idea to use multiprocessing when using gunicorn. I didn't realize this before because the tests don't actually run against gunicorn. There is however a better way of doing things than what we have now, which is to use Celery to manage the communication with the message broker. I'll close this PR and create a separate one using Celery.