Page MenuHomePhabricator

Dagster Celery K8s Job step executor
ClosedPublic

Authored by nate on Apr 17 2020, 3:48 AM.

Details

Summary

Depends on D2787.
Depends on D2811.

Test Plan

tested manually; added buildkite tests

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
nate edited the summary of this revision. (Show Details)Mon, May 4, 2:52 AM

is this still RFC status or is it ready to go? Would be good for @max to take a pass

.buildkite/images/docker/test_project/build.sh
53 ↗(On Diff #13070)

we want to keep this or was this from testing?

nate planned changes to this revision.Mon, May 4, 3:54 PM

Almost good to go, I need to fix py2 so planning changes

Harbormaster completed remote builds in B10684: Diff 13141.
nate retitled this revision from RFC: Dagster Celery K8s Job step executor to Dagster Celery K8s Job step executor.Mon, May 4, 9:49 PM
nate edited the test plan for this revision. (Show Details)
nate added a comment.Mon, May 4, 11:21 PM

At last, this is ready for review, after D2746 gets in

max added inline comments.Tue, May 5, 5:03 AM
python_modules/libraries/dagster-celery/dagster_celery/engine.py
26

not to be difficult but i would like to break this out into its own module, dagster_celery.tags

43–44

@alangenfeld maybe you can weigh in on whether it's worth trying to unify this with ActiveExecution, formalize some hooks, etc.

214–220

i think i agree, it'll reduce the chances of bad invocations / be an obvious place to do validation

python_modules/libraries/dagster-celery/dagster_celery/executor.py
7–8

why not just use a constant?

python_modules/libraries/dagster-celery/dagster_celery/tasks.py
79

for future-proofing, prefer bind=True

115

hmm... i think this might blow up unexpectedly in horrible ways long after we've forgotten about all this, like if someone is running DinD for completely different reasons but expecting the kubconfig_file param they passed explicitly to be respected. maybe a stricter check

245

this all seems very tenuous. do we need to autodetect this vs setting an explicit flag in an env variable or similar?

nate updated this revision to Diff 13236.Tue, May 5, 10:47 PM
nate marked 10 inline comments as done.
nate edited the test plan for this revision. (Show Details)

comments

python_modules/libraries/dagster-celery/dagster_celery/engine.py
26

cool, this sgtm!

214–220

yep - this exists now, see DagsterK8sJobConfig which wraps up the shared configuration around launching Dagster Jobs on K8s.

The job namespace and kubeconfig file ref are not included in that namedtuple, because the job definition is invariant to those two things. I'm open to adding another layer namedtuple above to bundle all three together if either of you feel strongly that such a thing should exist

python_modules/libraries/dagster-celery/dagster_celery/tasks.py
115

yeah, fair enough - I'm replacing this with the approach in D2787, will also align the implementation here with how we do it in the RunLauncher

117–121

for future reference: planning labels refresh after this lands as a separate diff

245

removed :)

jordanbramble added inline comments.
.buildkite/pipeline.py
282

obviously non-blocking, but we should probably use .format() since that is consistently used elsewhere.

python_modules/libraries/dagster-celery/conftest.py
8

Does this have to be specified when cluster provider is kind?

nate updated this revision to Diff 13343.Thu, May 7, 12:31 AM

missed some tag strings

nate updated this revision to Diff 13348.Thu, May 7, 1:41 AM

rebase

nate updated this revision to Diff 13354.Thu, May 7, 3:01 AM

speculative rebase on D2811 to fix celery test

nate updated this revision to Diff 13355.Thu, May 7, 3:34 AM

retrigger build

nate edited the summary of this revision. (Show Details)Thu, May 7, 3:49 AM
nate added a parent revision: D2811: Reduce Celery test runtime.
max resigned from this revision.Thu, May 7, 5:43 AM

this lgtm, i am resigning in favor of alex since he had objections previously

nate marked an inline comment as done.Thu, May 7, 3:29 PM
nate added inline comments.
.buildkite/pipeline.py
282

yeah good call - this line copied from k8s tests, but I will put up a follow-up diff - I'd like to clean up all of the k8s-related test configs across dagster-k8s, dagster-airflow, and dagster-celery after this

python_modules/libraries/dagster-celery/conftest.py
8

nope! and I've just mirrored these from the dagster-k8s configs since the pytest fixtures these are associated with are defined over there

alangenfeld accepted this revision.Thu, May 7, 3:57 PM

winclap

python_modules/libraries/dagster-celery/dagster_celery/engine.py
43–44

seems pretty decent as is - most of the stuff outside of [1] is celery specific

92–97

[1]

211

safer if this name reflects that its a serialized value, also consider using pack and unpack value wrapped in to / from dict methods like the instance_ref_dict

This revision is now accepted and ready to land.Thu, May 7, 3:57 PM
nate updated this revision to Diff 13415.Thu, May 7, 6:47 PM
nate marked an inline comment as done.

comments

This revision was automatically updated to reflect the committed changes.