Page MenuHomePhabricator

Simplify engine interface
ClosedPublic

Authored by max on Thu, Aug 1, 9:12 PM.

Details

Reviewers
schrockn
alangenfeld
natekupp
Group Reviewers
Restricted Project
Commits
R1:596d7c1f7bd4: Simplify engine interface
Summary

Preliminary to selecting engines through Dagit -- first make sure that we are using a single code path for all of our execution modalities.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

max created this revision.Thu, Aug 1, 9:12 PM
max updated this revision to Diff 3399.Mon, Aug 5, 5:04 PM

Update docs

max updated this revision to Diff 3400.Mon, Aug 5, 5:05 PM

Rebase

Harbormaster failed remote builds in B2713: Diff 3400!
max retitled this revision from Wip to Simplify engine interface.Mon, Aug 5, 5:21 PM
max edited the summary of this revision. (Show Details)
max added reviewers: Restricted Project, schrockn, alangenfeld.
max updated this revision to Diff 3404.Mon, Aug 5, 5:25 PM

Lint

can you update the summary with a little detail as to how these changes set us up for further improvement? I don't see any objectionable code changes here, but its also not immediately obvious where this is going.

python_modules/dagster-dask/dagster_dask_tests/test_execute.py
28–30

is there a reason to do it this way vs importing the pipeline directly?

max added a comment.Tue, Aug 6, 7:18 PM

Yep, the second phase eliminates executor_config from RunConfig entirely, specifying it through the environment dict. It's key for this that all of our execution modalities use a single code path.

python_modules/dagster-dask/dagster_dask_tests/test_execute.py
28–30

yes, only pipelines instantiated from handles can be executed across process boundaries

max edited the summary of this revision. (Show Details)Tue, Aug 6, 7:18 PM
natekupp accepted this revision.Tue, Aug 6, 9:30 PM
natekupp added a subscriber: natekupp.
natekupp added inline comments.
python_modules/dagster/dagster/core/system_config/objects.py
120

maybe check to ensure that config doesn't have multiple items since you're doing single_item here?

python_modules/dagster/dagster/utils/__init__.py
277

does this have test coverage? since you're promoting to utils might be nice to write a small test to demo what it does

This revision is now accepted and ready to land.Tue, Aug 6, 9:30 PM
max updated this revision to Diff 3469.Tue, Aug 6, 9:56 PM

Utils

max updated this revision to Diff 3470.Tue, Aug 6, 9:57 PM

Rebase

max added inline comments.Tue, Aug 6, 10:01 PM
python_modules/dagster/dagster/core/system_config/objects.py
120

this is actually what single_item does, somewhat horribly. i'll rename it.

python_modules/dagster/dagster/utils/__init__.py
277

yep

Harbormaster completed remote builds in B2771: Diff 3470.
This revision was automatically updated to reflect the committed changes.