Page MenuHomeElementl

[dagster-k8s] Have job image from executor config take precedence instead of erroring
ClosedPublic

Authored by sashank on Dec 14 2020, 5:52 PM.

Details

Summary

Instead of throwing an error, have a job image from executor config take precedence over a job image from a user code deployment. Also report an engine event to make it clear that this is happening.

Test Plan

integration
newline

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sashank edited the test plan for this revision. (Show Details)

can you test this? Manual is acceptable, integration test if you can come up with it

python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/launcher.py
167–175

i think you want to be using f"" strings here if you want {variable_name} to interpolate correctly, this is just going to print the exact string "...job_image {job_image_from_executor_config}..."

This revision now requires changes to proceed.Dec 16 2020, 11:02 PM

Oof this was a mistake cleaning up my old diffs. Thank you for remembering this diff

This revision now requires changes to proceed.Mar 22 2021, 4:03 PM

add tests

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

I moved this down here bc I'm guessing we always want this set, regardless fo where the job image came from

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

These two tests are a bit rough with the monkeypatch, but didn't really see another easy way to exercise the code path

alangenfeld added inline comments.
python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s_tests/test_launcher.py
236

maybe try to do a cleanup pass after - main thing to worry about are these tests slowing future eng down by failing for bad reasons

This revision is now accepted and ready to land.Mar 22 2021, 7:54 PM

final tox fixes to make dagster-test work...

This revision was landed with ongoing or failed builds.Mar 22 2021, 9:52 PM
This revision was automatically updated to reflect the committed changes.