Page MenuHomePhabricator

Refactor execute_step_with_structured_logs
AcceptedPublic

Authored by johann on Oct 12 2020, 8:01 PM.

Details

Summary

Rename and remove args that can be gathered from the pipeline_run

Test Plan

celery k8s integration

Diff Detail

Repository
R1 dagster
Branch
refactor_execute_step (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

johann retitled this revision from modify execute_step args to Refactor execute_step_with_structured_logs.Oct 12 2020, 8:12 PM
johann edited the summary of this revision. (Show Details)
johann edited the test plan for this revision. (Show Details)
johann added reviewers: alangenfeld, dgibson.
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 12 2020, 9:06 PM
Harbormaster failed remote builds in B19411: Diff 23587!

nice, this seems uncontroversial

This revision is now accepted and ready to land.Oct 13 2020, 5:19 PM

if someone upgrade their user code but forgets to update the celery workers - how does this fail? We don't promise cross version compat so i think this "breaking" change is probably ok - as long as it fails in a clear way that someone knows what to do about it

python_modules/dagster/dagster/cli/api.py
149

and i think if we just kept this around that points directly at the new execute_step one, we may be totally fine

python_modules/dagster/dagster/grpc/types.py
72–86

due to how serdes works, I don't think this will be a problem

rebase and add to other celery executors

Could I get another quick look at this before landing since it's been sitting around

if someone upgrade their user code but forgets to update the celery workers - how does this fail? We don't promise cross version compat so i think this "breaking" change is probably ok - as long as it fails in a clear way that someone knows what to do about it

Update user code -> run worker (former run coordinator) spins up with the new code, and makes a celery call with the new args -> task apply fails, raises error about signature

and i think if we just kept this around that points directly at the new execute_step one, we may be totally fine

I don't think this helps in the k8s case due to the run worker using the user code as well (user code -> celery -> user code, so we'll break on celery signature on the first hop). I think the case where it does help is with dagster-celery-docker, where the docker image could be newer than the celery worker calling it. If it's older than the worker calling it, we'd break since execute_step won't be available.

Add execute_step_with_structured_logs for back compat

still seems reasonable to me

CHANGES.md
1

merge issues

python_modules/dagster/dagster/cli/api.py
161–162

could do the same with execute_run now that that's gone

python_modules/dagster/dagster_tests/cli_tests/test_api_commands.py
80–81

something's not right here

given the "breaking change" adjacent potential - should we hold off on landing this til branch cut since were close (potentially after the next release)?

python_modules/dagster/dagster/cli/api.py
161–162

the execute_run_with... does have external callers - custom run launchers, so keep that in mind for that one

hold off on landing this til branch cut

sounds good to me