Page MenuHomePhabricator
Differential D5456

Improve error messages for celery-k8s launcher
ClosedPublic
Actions

Authored by sashank on Dec 7 2020, 5:56 PM.

Details

Reviewers
alangenfeld
nate
catherinewu
Commits
R1:84d5305ac23c: Improve error messages for celery-k8s launcher
Summary

This diff improves the error messages when using the celery-k8s executor + launcher, specifically for the error cases when:

  • The user specifies a job_image when using user-code deployments
  • The user tries to use the configured API for the executor
Test Plan

integration
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

sashank updated this revision to Diff 27129.Dec 7 2020, 5:56 PM
sashank created this revision.

up

python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py
177–182

When does this actually happen? Is this even a possible error case, or is this an internal check?

191–197

When does this actually happen? Is this even a possible error case, or is this an internal check?

sashank added inline comments.Dec 7 2020, 5:58 PM
python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py
177–182

Ignore this message, moved comment to correct spot below.

sashank published this revision for review.Dec 7 2020, 6:04 PM
catherinewu added inline comments.Dec 7 2020, 6:07 PM
python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py
211

i think we're currently being too strict and we actually want to support passing in a job_image even if the user has user code deployments. See https://github.com/dagster-io/dagster/issues/3286

Harbormaster failed remote builds in B22326: Diff 27128!Dec 7 2020, 6:13 PM
sashank planned changes to this revision.Dec 7 2020, 6:14 PM

Integration failures

Harbormaster failed remote builds in B22327: Diff 27129!Dec 7 2020, 6:16 PM
sashank updated this revision to Diff 27137.Dec 7 2020, 6:23 PM

up

Harbormaster failed remote builds in B22333: Diff 27137!Dec 7 2020, 6:40 PM
sashank planned changes to this revision.Dec 7 2020, 8:53 PM
sashank updated this revision to Diff 27419.Dec 10 2020, 4:35 PM

up

Harbormaster failed remote builds in B22555: Diff 27419!Dec 10 2020, 4:56 PM
catherinewu resigned from this revision.Dec 11 2020, 4:50 AM
sashank updated this revision to Diff 27714.Dec 14 2020, 5:11 PM

rebase past errors

alangenfeld accepted this revision.Dec 14 2020, 5:18 PM
alangenfeld added a subscriber: catherinewu.

I agree with @catherinewu that we should support a provided job image and maybe emit an engine event or log that we are overwriting the user code deploy image or something

that said this diff as is a strict improvement over the current error experience

This revision is now accepted and ready to land.Dec 14 2020, 5:18 PM
Harbormaster completed remote builds in B22787: Diff 27714.Dec 14 2020, 5:34 PM
sashank added inline comments.Dec 14 2020, 5:41 PM
python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py
211

I missed this comment, good call. Will add that in a follow up.

Closed by commit R1:84d5305ac23c: Improve error messages for celery-k8s launcher (authored by sashank). · Explain WhyDec 14 2020, 5:42 PM
This revision was automatically updated to reflect the committed changes.
sashank added a commit: R1:84d5305ac23c: Improve error messages for celery-k8s launcher.

Revision Contents
Changeset List

PathSize
python_modules/
libraries/
dagster-celery-k8s/
dagster_celery_k8s/
52 lines
dagster_celery_k8s_tests/
8 lines

Diff 27723

View Options

python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py

Loading...
View Options

python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s_tests/test_launcher.py

Loading...