Page MenuHomeElementl

added addresses to StepInput
ClosedPublic

Authored by cdecarolis on Oct 2 2020, 9:06 PM.

Details

Summary

Pass addresses to step input for use during input hydration.

Test Plan

Added test to versioned_execution_plan.py

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

theres nothing objectionable in here but I actually think a bigger more fleshed out diff that actually uses this newly added information would be preferable.

python_modules/dagster/dagster/core/execution/plan/objects.py
136–167

just looking at the class I would expect there to be a new enum entry for an input that loads from addresses

python_modules/dagster/dagster_tests/core_tests/test_versioned_execution_plan.py
61–77 ↗(On Diff #23207)

nit: this type of test is leaning towards ensuring a certain implementation versus ensuring a desired capability. Tests that encode too much about an expected implementation tend to only fail when someone is intentionally changing that implementation, and usually not in a helpful way. Thanks for coming to my TED talk.

theres nothing objectionable in here but I actually think a bigger more fleshed out diff that actually uses this newly added information would be preferable.

It's intended to support https://dagster.phacility.com/D4659. Is it preferred if I squash these two?

python_modules/dagster/dagster/core/execution/plan/objects.py
136–167

I got rid of this due to the case where you have "partially" memoized step inputs. If you have a step input that takes in inputs from multiple different steps, theoretically it should be possible for some of those to have resolved versions, and some not. This case convinced me to avoid the use of a new enum entry, and instead to bake the fxnality into the existing handling for the from output type. Curious as to your thoughts on that, maybe I'm wrong in assuming that this can happen.

python_modules/dagster/dagster_tests/core_tests/test_versioned_execution_plan.py
61–77 ↗(On Diff #23207)

That makes sense. Do you have any recommendations on how to test something like this? I was struggling to think of anything to test this part specifically outside of the context of execution (I guess the tests for the actual execution would arguably be enough).

Oh cool I didn't catch the stacked diff - I think in this specific case you could have folded them together but not a big deal. If you do depends on Dxxx in the summary for diffs that are stacked phabricator will pick up on it and display a stack section in the UI for all the diffs in the stack so recommend that.

python_modules/dagster/dagster/core/execution/plan/objects.py
136–167

ah interesting

python_modules/dagster/dagster_tests/core_tests/test_versioned_execution_plan.py
61–77 ↗(On Diff #23207)

ya i personally would have just referenced its use in the stacked diff in the test plan or something

This revision is now accepted and ready to land.Oct 5 2020, 3:59 PM
python_modules/dagster/dagster/core/execution/plan/objects.py
164

Are we able to set the value_type as well?

Removed implementation-specific test. Added type checking to values in address dict