User Details
- User Since
- Jul 6 2020, 12:49 PM (152 w, 1 d)
- Roles
- Disabled
Sep 8 2021
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
Aug 20 2021
Aug 19 2021
update after succesfully running
Aug 11 2021
(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
Aug 9 2021
Aug 4 2021
Aug 3 2021
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
Aug 2 2021
@dish before:
after: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
Aug 1 2021
Jul 31 2021
up
Jul 30 2021
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
Jul 29 2021
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.
Jul 28 2021
Jul 27 2021
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..