Page MenuHomePhabricator

Flesh out and revise deployment documentation
ClosedPublic

Authored by max on Feb 6 2020, 11:43 PM.

Details

Summary

Fixes and reorganization. Doesn't yet include k8s/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 6 2020, 11:43 PM
max edited the summary of this revision. (Show Details)Feb 7 2020, 10:34 PM
max added reviewers: nate, sashank, prha.
schrockn added inline comments.Feb 7 2020, 11:20 PM
docs/sections/deploying/airflow.rst
13–16

this is great. perhaps point to celery?

25–26

I might emphasize that it actually produces a DAG with a *single* operator type. Maybe even link to as a citation for inspiration

https://medium.com/bluecore-engineering/were-all-using-airflow-wrong-and-how-to-fix-it-a56f14cb0753

29–32

I think we should expose the "operator" argument on make_airflow_dag and note that one can also use your own operator

188

are we concerned with this becoming out-of-date. e.g. if they move the class

fantastic work max

docs/sections/api/apidocs/dagster_aws.rst
1

We might consider making it clear that the types like S3FileCache and s3_system_storage are quite orthogonal to the CLI. They could almost be two wholly separate modules

docs/sections/deploying/aws.rst
9–15

Maybe we go further and say this this is no way meant to replace systems like terraform, pulumi etc. This is essentially for deploying demos and proofs of concept

105–108

I think our language should be stronger here. In order to parallelize computation you *have* to use an persistent intermediate store

docs/sections/deploying/dask.rst
1–21

Would love to have some language to further hammer home that this is using dask for *orchestration* rather than a compute substrate for your data (you are getting to this in the last para for sure)

94–107

I think it would be good to make clear the the dict is just passed straight through to the dask APIs and maybe even point to them

112

only a persistent intermediate storage. s3 is an example. make clear this is multi-cloud.

also this makes it sounds like run storage is in s3

docs/sections/deploying/gcp.rst
34–36

s/S3/GCS

docs/sections/deploying/index.rst
17–18

not following this sentence

docs/sections/deploying/instance.rst
34–58

this is great documentation

schrockn resigned from this revision.Feb 7 2020, 11:52 PM

I'm very pleased with this overall. I'm going to resign to get it off my queue.

nate added inline comments.Feb 10 2020, 4:55 PM
docs/sections/deploying/airflow.rst
67

might be worth talking about how someone might set up a CI/CD pipeline to do this, and our expectations about how Dagster code is deployed in an Airflow context

102–103

hmm, maybe containerize your *Dagster repository? I had to do a double-take on this sentence because I was thinking about a Docker repository. Wonder if we should also comment on deploy process here as well?

docs/sections/deploying/dask.rst
1–21

This idea seems like a foundational concept that we should introduce at a top level of the docs somewhere, it's important and applicable across all of these execution integrations

docs/sections/deploying/gcp.rst
34–36

Might be worth foregrounding at the top of these documents a bulleted list of requirements for AWS and GCP deployments? Like:

GCP Requirements:
• GCE VM for Dagit
• (Optional) GCS bucket for intermediate storage
• (Optional) Cloud SQL database for run and events storage

I guess these could also link down to the relevant sections. I'm noticing it's a little tough right now to scan and find all the requirements in the body of the docs. Thoughts?

docs/sections/deploying/index.rst
22

Hmm, can we leave some kind of quick start on this page? Maybe I'm over-worried about this, but I'd rather not ask our users to learn this much about the system before we get them up and running.

As a comparison, I remember being happy with https://airflow.apache.org/docs/stable/start.html as making it super easy to get started

docs/sections/deploying/instance.rst
62

missing backtick

max added inline comments.Feb 10 2020, 9:09 PM
docs/sections/api/apidocs/dagster_aws.rst
1

yep

docs/sections/deploying/airflow.rst
13–16

yep, i'll add that in the follow-on k8s/celery diff

25–26

yep

29–32

this is a good idea but will require a little refactor, i will file an issue

67

yep

102–103

yep

188

it's a link so yes

docs/sections/deploying/aws.rst
9–15

yep

105–108

ok

docs/sections/deploying/dask.rst
94–107

yep

112

yep

docs/sections/deploying/gcp.rst
34–36

yeah, i'd like to consolidate the AWS and GCP docs into a single "Cloud deployments" doc and will do this as part of that

34–36

yep

docs/sections/deploying/index.rst
22

the quick start is just dagit?

winclap

docs/sections/deploying/dagster.yaml
5–34

update these to use postgres_db:

docs/sections/deploying/instance.rst
68–71

the SQLite databases will be in-memory rather than persisted to disk

I think we actually just use a class backed by in memory dicts but not sure how important it is to get this right. I think

will use a temporary directory that is cleaned up when the process exits

is the important bit.

maaaaaaaybe worth calling out that the dagit parent watcher process will keep maintain the temp dir across restarts of the inner dagit-cli process. Maybe just in a separate Note: section or something

120

maybe choose a different placeholder than 0

python_modules/dagit/dagit/cli.py
114

? i assume this isnt a behavior change

python_modules/libraries/dagster-aws/dagster_aws/s3/system_storage.py
47–48

might want to add a ... or <value> marker to indicate these need to be filled

alangenfeld accepted this revision.Feb 10 2020, 9:30 PM

I think this is close - fire at will

This revision is now accepted and ready to land.Feb 10 2020, 9:30 PM
max updated this revision to Diff 9465.Feb 10 2020, 10:01 PM
max marked 24 inline comments as done.

Up

max added inline comments.Feb 10 2020, 10:04 PM
docs/sections/deploying/dagster.yaml
5–34

yep

docs/sections/deploying/instance.rst
68–71

hmm, people do not know about dagit's process hierarchy tho

120

yep

python_modules/dagit/dagit/cli.py
114

this is to make everything play nicely with the sphinx click tools

python_modules/libraries/dagster-aws/dagster_aws/s3/system_storage.py
47–48

yep

alangenfeld added inline comments.Feb 10 2020, 10:11 PM
docs/sections/deploying/instance.rst
68–71

ya its a real stretch - there exists some scenario where someone would like to know whats going on - but I guess thats what the instance info on hover thing in dagit is for - maybe thats worth mentioning somewhere.

This revision was automatically updated to reflect the committed changes.