Page MenuHomePhabricator

Document the Celery executor
ClosedPublic

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

Looking nice. Thanks for doing this.

docs/sections/api/apidocs/dagster_gcp.rst
2–43

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
74

In production, more configuration is required

80

may want to note that this is typically at DAGSTER_HOME

86

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.Tue, Feb 11, 10:36 PM
alangenfeld added inline comments.Tue, Feb 11, 10:37 PM
docs/sections/deploying/celery.rst
9–10

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.Tue, Feb 11, 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
33–38

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

40–41

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

52

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

54

Now can

Wording

54

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"

56–61

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.

71–72

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.Wed, Feb 12, 12:05 AM
docs/sections/deploying/celery.rst
68

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

70

worker's -> celery worker

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

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.Wed, Feb 12, 1:20 AM
max updated this revision to Diff 9571.Wed, Feb 12, 6:12 AM

Updating

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

Rebase

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

looks great. merge away