Page MenuHomePhabricator

[9/n] Make StepInputSource use snapshots
AbandonedPublic

Authored by dgibson on Jan 21 2021, 11:08 PM.

Details

Reviewers
alangenfeld
Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
rmpipelinedef9
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

dgibson retitled this revision from Make StepInputSource use snapshots to [9/n] Make StepInputSource use snapshots.Jan 21 2021, 11:12 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 22 2021, 12:16 AM
Harbormaster failed remote builds in B24673: Diff 30048!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 22 2021, 5:02 AM
Harbormaster failed remote builds in B24693: Diff 30071!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 22 2021, 6:04 AM
Harbormaster failed remote builds in B24695: Diff 30073!
alangenfeld added inline comments.
python_modules/dagster/dagster/core/execution/plan/inputs.py
85–88

nit: might be cleaner to just hoist compute_version calc up off this class

92

nit: think just solid_handle is clearer (to me)

python_modules/dagster/dagster/core/execution/plan/plan.py
251–328

same as prev diff - would like to see this get cleanedup in stack before stack lands

python_modules/dagster/dagster/core/snap/dagster_types.py
29–98

shouldn't need these in the snapshot - I could see required_resource_keys maybe being something we would want to display in dagit / know in host, but loader_required_resource_keys seems very dubious

python_modules/dagster/dagster/core/snap/solid.py
296

also not needed - at least for what you are trying to accomplish - we should grab the real defs for resource init path

This revision now requires changes to proceed.Jan 22 2021, 10:27 PM