Page MenuHomePhabricator

Core dagster_celery engine
ClosedPublic

Authored by max on Dec 3 2019, 7:01 PM.

Details

Summary

This diff makes it possible to execute pipelines in parallel through Dagit using a Celery executor.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

great overall. definitely want to rely less on hardcoded config and more on the semantic properties of our internal abstractions (e.g. is_persistent)

In general I think we should be quite wary about just jamming stuff in environment dicts. Every time I see that I interpret it as a deficiency in our core APIs tbh.

python_modules/dagster-celery/dagster_celery/engine.py
35

we should be checking is_persistent on the SystemStorageDefinition here

45

we should be checking is_persistent on the SystemStorageDefinition here

56

this should be in terms of is_persitent

105

add issue number to track at a min

python_modules/dagster-celery/dagster_celery_tests/engine_config.yaml
8

does this parse

python_modules/dagster-celery/dagster_celery_tests/test_cli.py
15

worth a comment?

python_modules/dagster/dagster/cli/load_handle.py
98

why the change here

This revision now requires changes to proceed.Dec 10 2019, 10:24 PM
max updated this revision to Diff 7515.Dec 10 2019, 10:26 PM

Add ping tasks

max updated this revision to Diff 7518.Dec 10 2019, 10:40 PM

Drop clever checks

max added inline comments.Dec 10 2019, 10:50 PM
python_modules/dagster-celery/dagster_celery/engine.py
35

yeah, i'm going to drop these checks completely - they are covered by CeleryConfig.check_requirements

45

dropping these checks

56

same

64

ok

105

i don't think it makes sense to add issues for all of the tk stuff on this engine?

python_modules/dagster-celery/dagster_celery_tests/test_cli.py
15

ok

python_modules/dagster-celery/setup.py
27

yup

python_modules/dagster/dagster/cli/load_handle.py
98

are you asking why abspath?

schrockn resigned from this revision.Dec 10 2019, 10:55 PM

this looks good to me. really exciting stuff. will let @alangenfeld or @nate pull the final trigger on this one!

Harbormaster failed remote builds in B6063: Diff 7514!
Harbormaster failed remote builds in B6068: Diff 7519!
Harbormaster failed remote builds in B6064: Diff 7515!
Harbormaster failed remote builds in B6069: Diff 7520!
max updated this revision to Diff 7534.Dec 10 2019, 11:26 PM

Arggh

max updated this revision to Diff 7550.Dec 11 2019, 12:14 AM

Rebase

max updated this revision to Diff 7561.Dec 11 2019, 12:35 AM

Skip CLI tests on buildkite

max updated this revision to Diff 7563.Dec 11 2019, 1:15 AM

Skip tests on buildkite

max updated this revision to Diff 7565.Dec 11 2019, 1:38 AM

Debug

Harbormaster failed remote builds in B6105: Diff 7563!
Harbormaster failed remote builds in B6103: Diff 7561!
max updated this revision to Diff 7566.Dec 11 2019, 1:43 AM

undebug

max updated this revision to Diff 7571.Dec 11 2019, 5:26 PM

debug

max updated this revision to Diff 7572.Dec 11 2019, 5:33 PM

Fix tests

max updated this revision to Diff 7584.Dec 11 2019, 7:19 PM

Rebase

max updated this revision to Diff 7587.Dec 11 2019, 7:34 PM

coveragerc

alangenfeld accepted this revision.Dec 12 2019, 6:31 PM
alangenfeld added inline comments.
python_modules/dagster-celery/README.md
65–78

back on my "todo items/lists in code are never kept up to date" bullshit - the risk here would be this becoming stale and someone taking this as the source of truth

python_modules/dagster-celery/dagster_celery/config.py
13–15

hackerman

python_modules/dagster-celery/dagster_celery/tasks.py
33

*

This revision is now accepted and ready to land.Dec 12 2019, 6:31 PM
max added inline comments.Dec 13 2019, 5:12 AM
python_modules/dagster-celery/README.md
65–78

ok ok

max updated this revision to Diff 7669.Dec 13 2019, 5:31 AM

rmtodo

max updated this revision to Diff 7672.Dec 13 2019, 5:46 AM

Rebase

This revision was automatically updated to reflect the committed changes.