Page MenuHomePhabricator
Differential D4754

Refactor execute_step_with_structured_logs
AcceptedPublic
Actions

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

Details

Reviewers
alangenfeld
dgibson
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 created this revision.Mon, Oct 12, 8:01 PM
johann retitled this revision from modify execute_step args to Refactor execute_step_with_structured_logs.Mon, Oct 12, 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.
johann updated this revision to Diff 23587.Mon, Oct 12, 8:23 PM

typos

Harbormaster failed remote builds in B19410: Diff 23586!Mon, Oct 12, 8:42 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Oct 12, 9:06 PM
Harbormaster failed remote builds in B19411: Diff 23587!
johann updated this revision to Diff 23609.Tue, Oct 13, 3:35 PM

up

johann updated this revision to Diff 23610.Tue, Oct 13, 3:53 PM

up

Harbormaster failed remote builds in B19428: Diff 23609!Tue, Oct 13, 4:12 PM
johann updated this revision to Diff 23613.Tue, Oct 13, 4:21 PM

up

Harbormaster failed remote builds in B19429: Diff 23610!Tue, Oct 13, 4:28 PM
johann updated this revision to Diff 23621.Tue, Oct 13, 4:51 PM

up

Harbormaster failed remote builds in B19432: Diff 23613!Tue, Oct 13, 4:59 PM
Harbormaster completed remote builds in B19439: Diff 23621.Tue, Oct 13, 5:13 PM
johann requested review of this revision.Tue, Oct 13, 5:13 PM
dgibson accepted this revision.Tue, Oct 13, 5:19 PM

nice, this seems uncontroversial

This revision is now accepted and ready to land.Tue, Oct 13, 5:19 PM
johann updated this revision to Diff 23936.Fri, Oct 16, 4:31 PM

rebase

Harbormaster failed remote builds in B19720: Diff 23936!Fri, Oct 16, 4:52 PM
alangenfeld added a comment.Fri, Oct 16, 7:36 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

alangenfeld added inline comments.Fri, Oct 16, 7:38 PM
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–66

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

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
cli/
23 lines
core/
execution/
8 lines
grpc/
10 lines
dagster_tests/
cli_tests/
11 lines
libraries/
dagster-celery-k8s/
dagster_celery_k8s/
95 lines

Diff 23936

View Options

python_modules/dagster/dagster/cli/api.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/api.py

Loading...
View Options

python_modules/dagster/dagster/grpc/types.py

Loading...
View Options

python_modules/dagster/dagster_tests/cli_tests/test_api_commands.py

Loading...
View Options

python_modules/libraries/dagster-celery-k8s/dagster_celery_k8s/executor.py

Loading...