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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
718–724

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
714–724

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
714–724

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