Page MenuHomePhabricator

Multiqueue execution
ClosedPublic

Authored by max on Dec 18 2019, 8:19 PM.

Details

Summary

POC for multiqueue execution in Celery engine.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

max created this revision.Dec 18 2019, 8:19 PM
schrockn added inline comments.Dec 18 2019, 8:44 PM
python_modules/dagster-celery/dagster_celery_tests/test_queues.py
26

we really have no thesis about why this is happening?

schrockn added inline comments.Dec 18 2019, 8:46 PM
python_modules/dagster-celery/dagster_celery/engine.py
103–104

forgot to comment on this in last diff, but can we this comment more useful?

schrockn requested changes to this revision.Dec 19 2019, 12:24 AM

This seems solid i just think we need to be able to have this stuff under ci

This revision now requires changes to proceed.Dec 19 2019, 12:24 AM
schrockn accepted this revision.Dec 19 2019, 2:57 AM

k let's move forward.

for posterity's sake I think we should figure out a way to locally test this without any sort of docker dependency at all. with multiple processes and a persistence queue somewhere it should be possible. I'm not a celery expert by any means but they certainly claim to have a local development story in their documentation. engine.py should have code coverage without a docker image.

This revision is now accepted and ready to land.Dec 19 2019, 2:57 AM
max added a comment.Dec 19 2019, 3:05 AM

here is coverage in the status quo (running with no queue):

dagster_celery/engine.py      62      2     26      3    94%   83-86, 80->79, 92->91, 121->120
dagster_celery/tasks.py       29      1      4      1    94%   51, 50->51

in my experience, the most dangerous parts of a system like this are never in user code

python_modules/dagster-celery/dagster_celery_tests/test_queues.py
26

no, the test setup works fine in a local integration image container

max updated this revision to Diff 8017.Dec 19 2019, 6:24 PM

Rebase

schrockn accepted this revision.Dec 19 2019, 6:31 PM
schrockn added inline comments.
python_modules/dagster-celery/dagster_celery_tests/test_queues.py
66

test for metadata explictly?

max added inline comments.Dec 19 2019, 9:23 PM
python_modules/dagster-celery/dagster_celery_tests/test_queues.py
66

sure

max updated this revision to Diff 8047.Dec 19 2019, 9:24 PM

Explicitly test metadata

This revision was automatically updated to reflect the committed changes.