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

Integration
foo <- newline necessary to trigger tests?

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
400

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
52–56

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 ↗(On Diff #25304)

merge issues

python_modules/dagster/dagster/cli/api.py
412–413

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

python_modules/dagster/dagster_tests/cli_tests/test_api_commands.py
83–84

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
412–413

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

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