Page MenuHomeElementl
Differential D5712

[Issue 3463] Remove versioning code paths from main execution path.
ClosedPublic
Actions

Authored by cdecarolis on Dec 21 2020, 4:32 PM.

Details

Reviewers
schrockn
sandyryza
yuhan
Commits
R1:d37ba2504bdf: [Issue 3463] Remove versioning code paths from main execution path.
Summary

In reference to issue https://github.com/dagster-io/dagster/issues/3463.
We re-compute the execution plan at every step for the purpose of computing version information. This diff hides that computation behind an experimental tag s.t. no normal runs will have version information computed.
I verified that the version computation code paths were never hit after this change in the affected tests. This change results in a >50% improvement in the speed of test_composites.py::test_composite_basic_execution.

Test Plan

Unit tests. Verified manually that versioning code paths were never hit on a normal run.

Before change: 22s for test_composites.py::test_composite_basic_execution.
After change: 9s for test_composites.py::test_composite_basic_execution.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cdecarolis created this revision.Dec 21 2020, 4:32 PM
cdecarolis edited the summary of this revision. (Show Details)Dec 21 2020, 4:42 PM
cdecarolis edited the test plan for this revision. (Show Details)
cdecarolis edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 21 2020, 4:47 PM
Harbormaster failed remote builds in B23253: Diff 28274!
cdecarolis updated this revision to Diff 28286.Dec 21 2020, 5:35 PM

Up

Harbormaster completed remote builds in B23260: Diff 28286.Dec 21 2020, 5:49 PM
cdecarolis requested review of this revision.Dec 21 2020, 5:49 PM
schrockn added a comment.Dec 21 2020, 8:01 PM

Since you already have the profiling set up, do you have a notion what the hot paths are for the remaining 9 seconds?

schrockn accepted this revision.Dec 21 2020, 8:02 PM
This revision is now accepted and ready to land.Dec 21 2020, 8:02 PM
cdecarolis added a comment.Dec 21 2020, 8:51 PM
In D5712#154253, @schrockn wrote:

Since you already have the profiling set up, do you have a notion what the hot paths are for the remaining 9 seconds?

I didn't get too great of a picture with the work I did for this, but I'll investigate further and follow up.

Closed by commit R1:d37ba2504bdf: [Issue 3463] Remove versioning code paths from main execution path. (authored by Chris DeCarolis <cdguitar817@gmail.com>). · Explain WhyDec 21 2020, 8:52 PM
This revision was automatically updated to reflect the committed changes.
Chris DeCarolis <cdguitar817@gmail.com> added a commit: R1:d37ba2504bdf: [Issue 3463] Remove versioning code paths from main execution path..

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
core/
execution/
context/
5 lines
plan/
7 lines
dagster_tests/
core_tests/
execution_tests/
4 lines
6 lines

Diff 28310

View Options

python_modules/dagster/dagster/core/execution/context/system.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/plan/execute_step.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/execution_tests/memoized_dev_loop_pipeline.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_versioned_execution_plan.py

Loading...