Page MenuHomePhabricator

Improve error messages for celery-k8s launcher
ClosedPublic

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

Details

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 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?

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
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

Integration failures

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
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.