see inline - esp the contextmanager thing. I'll sleep easier when this passes your new back-compat integration tests, and you'll want to have an internal diff ready to go before landing this i think

update after succesfully running

(was more relevant when it would replace the launcher config when it was set though - since in that case None was distinct from [])

@alangenfeld or @rexledesma if you wouldn't mind reviewing this since johann is out that week I would be very grateful

up

johann's feedback, beefed up the test coverage quite a bit

I think @alangenfeld is in the process of unearthing a better fix here (fixing the root cause of the deadlock vs. preventing errors from getting thrown)

tests in https://dagster.phacility.com/D9193 - lets make sure we have a change ready for internal though if this is breaking there

this looks good, one big thing that was missing from the diff this is patched on is python tests though - I may be able to tackle that this morning, given that permissions are fairly sensitive i think its important to have some coverage there

@dish before:

up

mypy

i'm not actually sure this is right despite fixing the hangs (in that it will cause batches of logs to just be skipped - if there was some way to still raise the exception, but give the observable time to clean up using its standard ways of doing that, that would be ideal)

step failure event with no serialized error info: https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/libraries/dagster-k8s/dagster_k8s/executor.py?L233:42

up

up

i like the idea of implementing the k8s termination check an a way that's akin to the other launchers, vs. affecting the other launchers

as per convo on the internal diff I think we're going to end up wanting to use ExternalXXXOrigins for this

add test case, make logic clearer

optionally something in the run launcher integration test that makes use of the env_vars field would be nice for e2e coverage, but we do have integration tests on the various pieces so that seems pretty good

sweet, let's get this into the release today for hebo.

Or I wonder if we had the same issue in the non-backfill case.. I’ll need to check

No worries. If there’s a test you’re adding to backfills for the thing you’re working on that caught this, that would break if I ever tried to I revert this later, that would be cool.

this looks good, but is back-compat a concern? if you upgrade dagit and don't upgrade the server will things break?

lint

up

back to queue since this is a much more comprehensive change now

add test case

up

up

up

up

up

up

up

add the rest of fields, which surfaces a bit of image pull policy inconsistency with the celery k8s config / run launcher..