Page MenuHomePhabricator

[RFC] requestPipelineExecution
AcceptedPublic

Authored by alangenfeld on Tue, Nov 5, 11:10 PM.

Details

Reviewers
max
Group Reviewers
Restricted Project
Summary

Change the API for pipeline execution to open up the door for pluggable execution managers / run queues.

  • startPipelineExecution -> requestPipelineExecution
    • new response types based on whether the run was started, requested, or completed. Not too attached to completed in the long term but it does work well in testing.
  • PipleinRunStatus NOT_STARTED -> REQUESTED
Test Plan

unit tests

Diff Detail

Repository
R1 dagster
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

alangenfeld created this revision.Tue, Nov 5, 11:10 PM
alangenfeld updated this revision to Diff 6266.Tue, Nov 5, 11:13 PM

clean up search and replaces gone wild

alangenfeld added inline comments.Wed, Nov 6, 4:47 PM
python_modules/dagster/dagster/core/storage/pipeline_run.py
11

back compat with reading from DB needs to get implemented or we decide that this rename is not worth the hassle

max added a subscriber: max.Wed, Nov 6, 5:33 PM
max added inline comments.
python_modules/dagster/dagster/core/storage/pipeline_run.py
11

we can do this pretty easily with https://alembic.sqlalchemy.org/en/latest/

max accepted this revision.Wed, Nov 6, 5:34 PM

feels right to me

This revision is now accepted and ready to land.Wed, Nov 6, 5:34 PM
prha added a subscriber: prha.Wed, Nov 6, 6:05 PM

requestPipelineExecution feels pretty good to me!

python_modules/dagster-graphql/dagster_graphql/implementation/execution.py
138

Nit: The name start_scheduled_execution might now be misleading.

I think the alternative request_scheduled_execution seems like it would mean something completely different (requesting the scheduling vs requesting the execution).

Is something like request_execution_from_schedule or something like that too awkward?

python_modules/dagster/dagster/core/storage/pipeline_run.py
11

I'm biased towards tackling the backwards compatibility sooner rather than later...

lgtm

python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_execution_manager.py
152–153

what is going on here

@sashank what do you think we should call the scheduler mutation?

python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_execution_manager.py
152–153

just driving the iterator to completion - stack overflow says the tryhard way is to do collections.deque(iterator, maxlen=0)

prha added inline comments.Wed, Nov 6, 9:11 PM
python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_execution_manager.py
152–153

did we use to have an execute_run in the API? might be worth while to have this wrapper in the api