Page MenuHomeElementl

[memoization improvements 1/n] get rid of step_keys_to_execute parameter on create_run_for_pipeline, simplify implementation of create_run_for_pipeline.
ClosedPublic

Authored by cdecarolis on Jul 16 2021, 2:55 PM.

Details

Summary

We never actually use the step_keys_to_execute_paramater on create_run_for_pipeline in any significant way, and removing it allows for singificant code simplification.

Test Plan

switched around unit tests where necessary.

Diff Detail

Repository
R1 dagster
Branch
create_pipeline_run_refactor
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 16 2021, 3:49 PM
Harbormaster failed remote builds in B33802: Diff 41743!

Preserve existing functionality (for now)

python_modules/dagster/dagster/core/instance/__init__.py
700–706

can totally get rid of this, I have that coming in a follow up with a larger treatment of step_keys_to_execute in general.

cdecarolis retitled this revision from get rid of step_keys_to_execute parameter on create_run_for_pipeline, simplify implementation of create_run_for_pipeline. to [memoization improvements 1/n] get rid of step_keys_to_execute parameter on create_run_for_pipeline, simplify implementation of create_run_for_pipeline..Jul 16 2021, 9:25 PM
python_modules/dagster/dagster/core/instance/__init__.py
696–706

yeah im confused by whats happening here - would this make more sense as part of the memoization block above ? Its not clear to me why were always doing this check

gonna round trip this one since im a little weary of that new check run for all creates

This revision now requires changes to proceed.Jul 20 2021, 8:02 PM
python_modules/dagster/dagster/core/instance/__init__.py
696–706

Right - so there's some serious weirdness here. Essentially, execution_plan.step_keys_to_execute does not have the same meaning as step_keys_to_execute that we pass around independently. In the step_keys_to_execute argument that we independently pass around, None means every step. But execution_plan.step_keys_to_execute does not accept a None value, and instead every step means every step.

There are two routes, as I see it. The less-friction one is what I have done above, essentially translating execution_plan.step_keys_to_execute back into the freeformed one with the None assumption. The other one would be to make execution_plan.step_keys_to_execute conform to the way we use it free-standing.

At the very least, I'm pretty sure that we need to keep the None == ever step assumption around for the free-standing one, so we don't break people's run history.

Does that somewhat clear it up / do you have thoughts on which approach you prefer?

Get rid of universal step_keys_to_execute check

thanks! very clean change now

This revision is now accepted and ready to land.Jul 22 2021, 4:33 PM