Page MenuHomePhabricator

Document the Celery executor
ClosedPublic

Authored by max on Feb 11 2020, 9:29 PM.

Details

Summary

Adds basic docs for execution using dagster-celery.

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.Feb 11 2020, 9:29 PM
schrockn requested changes to this revision.Feb 11 2020, 10:36 PM

Looking nice. Thanks for doing this.

docs/sections/api/apidocs/dagster_gcp.rst
1–42

let's omit BigQuery and Dataproc stuff for now. i want to predicate the inclusion of document on an audit of these libraries. I'm not convinced that these are patterns that we want the ecosystem to cargo cult

docs/sections/deploying/celery.rst
73

In production, more configuration is required

79

may want to note that this is typically at DAGSTER_HOME

85

I would with why one has to do this.

E.g.

Because data is passed between solids potentially running on different nodes, you must use storage that is accessible from all the nodes in the celery cluster. Examples include s3 or gcs. An appropriate system_storage -- such as s3_system_storage or gcs_strorage -- must be available in the ModeDefinition

docs/sections/deploying/k8s.rst
2–19 ↗(On Diff #9536)

seems incomplete?

This revision now requires changes to proceed.Feb 11 2020, 10:36 PM
alangenfeld added inline comments.Feb 11 2020, 10:37 PM
docs/sections/deploying/celery.rst
8–9

levers -> leverages?

docs/sections/deploying/k8s.rst
1–19 ↗(On Diff #9536)

this feels like scratch work? clean up and handle in other k8s focused diff?

python_modules/dagster-celery/dagster_celery/executor.py
70–76

feel like this should be documented as well as its siblings

max updated this revision to Diff 9557.Feb 11 2020, 11:52 PM
max marked 7 inline comments as done.
max removed a reviewer: sashank.

Respond to comments

Just a brain dump of questions I had while reading this

docs/sections/deploying/celery.rst
32–37

Can we link to: http://docs.celeryproject.org/en/latest/getting-started/brokers/ here to provide instructions on how to use other brokers + setups straight from the source

39–40

From the same directory

Why does it need to be from the same directory? It would be good to highlight what implicit dependence there is on the directory I run the command from

51

I think running it like this wouldn't use the celery executor? If so, we should clarify that this is just a sanity check.

53

Now can

Wording

53

Also, we should explicitly explain how adding the config below enables the celery executor. Also, it might be a good idea to explain why we need `storage: filesystem"

55–60

It looks like I need to do this from dagit. Probably should make that explicit, or show the command to execute w/ config from the command line.

70–71

Don't really understand what you mean by the code is available to both Dagit and the worker, and why running the worker in the same directory would enable this.

sashank added inline comments.Feb 12 2020, 12:05 AM
docs/sections/deploying/celery.rst
67

By running a single worker -> By running a single celery worker

69

worker's -> celery worker

max marked 4 inline comments as done.Feb 12 2020, 12:43 AM
max added inline comments.
docs/sections/api/apidocs/dagster_gcp.rst
1–42

ok

docs/sections/deploying/k8s.rst
2–19 ↗(On Diff #9536)

yep sorry, should be in next diff

Harbormaster failed remote builds in B7767: Diff 9564!
alangenfeld accepted this revision.Feb 12 2020, 1:20 AM
max updated this revision to Diff 9571.Feb 12 2020, 6:12 AM

Updating

max added a reviewer: sashank.Feb 12 2020, 5:01 PM
max updated this revision to Diff 9581.Feb 12 2020, 6:07 PM

Rebase

max removed a reviewer: schrockn.Feb 12 2020, 6:16 PM
This revision is now accepted and ready to land.Feb 12 2020, 6:16 PM
This revision was automatically updated to reflect the committed changes.

looks great. merge away