Page MenuHomePhabricator

[run-queue-5] Default runs coordinator (sends to external process)
ClosedPublic

Authored by johann on Oct 27 2020, 2:41 AM.

Details

Summary

Adds the interface to the external runs coordinator

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

change from MockedRunLauncher to MockedRunQueuer

johann retitled this revision from [run-queue-5] Default run queuer (sends to external process) to [run-queue-5] Default runs coordinator (sends to external process).Oct 29 2020, 8:26 PM
johann edited the summary of this revision. (Show Details)
johann added reviewers: alangenfeld, dgibson.
dgibson requested changes to this revision.Oct 30 2020, 1:41 PM

other than the name this seems good to me, see inline. We can also do a single diff at the end of this stack where we do a big codemod to make all the final naming decisions, since we're going back and forth so much and renaming through a stack is a pain

python_modules/dagster/dagster/core/runs_coordinator/default_runs_coordinator.py
14 ↗(On Diff #24841)

wait is this the default? Isn't the one that launches immediately the default? The one where things 'just work' without extra deployment and config should be the default IMO (and the one that we're setting as the default in the instance)

This revision now requires changes to proceed.Oct 30 2020, 1:41 PM

few small things inline

python_modules/dagster/dagster/core/runs_coordinator/queued_runs_coordinator.py
38–41

this could go on the base class too?

44–45

realizing now that this is mildly confusing (even though it also exists on the runlauncher) since there is a separate InstanceRef class

49

opt_inst_param doesn't seem right here even if this run coordinator happens to not use it

52–65

should this be a static pipeline_enqueued method on DagsterEvent like the other event creation methods?

66

seeing it in action now there's something strange about the fact that this is what triggers the status change / implements the actual business logic of the queuer, but it does appear to be the convention with other events, so meh

71

return False

This revision is now accepted and ready to land.Mon, Nov 2, 4:06 PM
johann added inline comments.
python_modules/dagster/dagster/core/runs_coordinator/queued_runs_coordinator.py
38–41

Currently they're both the same but eventually I think it's possible queued would want to setup a connection with the heartbeat table or something like that. Then we'd need to do super.initialize. I think I prefer keeping the definition here, but don't have a strong opinion

44–45

_instance_weak_ref maybe

52–65

Holding off on this because the other methods use SystemExecutionContext which we don't have yet, so we don't have a clean object to pass with all the info. I'll refactor this in the future because stuff like timestamp=time.time() really doesn't belong here

66

Agreed on meh

71

Going to save these for the future cancellation diff. There currently are no callsites to it (Dagit's still going to the launcher), which this helps me keep clear

johann marked 2 inline comments as done.

rebase

This revision was landed with ongoing or failed builds.Tue, Nov 3, 1:49 AM
This revision was automatically updated to reflect the committed changes.