Page MenuHomePhabricator

load dagit settings for queueing execution manager from dagster.yaml
ClosedPublic

Authored by prha on Wed, Jan 8, 6:44 PM.

Details

Test Plan

configured dagster.yaml with:

run_launcher:
  module: dagster_graphql.launcher
  class: RemoteDagitRunLauncher
  config:
    address: http://localhost:3333
dagit:
  execution_manager:
    max_concurrent_runs: 2

and ran jobs scheduled through both dagit and the backfill cli

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

prha created this revision.Wed, Jan 8, 6:44 PM
prha updated this revision to Diff 8453.Wed, Jan 8, 6:53 PM

fix missed callsite

prha edited the test plan for this revision. (Show Details)Wed, Jan 8, 8:11 PM
prha added reviewers: alangenfeld, schrockn.

hm this seems a little odd - why are dagit settings flowing through next to the instance - shouldn't be part of the instance?

prha added a comment.Thu, Jan 9, 12:44 AM

that's a good question.... I always thought of dagit as being orthogonal to the dagster instance, but it makes things a lot simpler just to have this be on the instance config

alangenfeld requested changes to this revision.Thu, Jan 9, 3:50 PM

I think it makes sense to opt for the simpler / lower code surface area approach for now and we can refactor it out later when we have a clearer sense of things

This revision now requires changes to proceed.Thu, Jan 9, 3:50 PM
prha updated this revision to Diff 8517.Thu, Jan 9, 7:13 PM

Adds a dagit settings section to the instance config... This keeps the dagit settings as a dictionary instead of a configurable class.

dagit:
  execution_manager:
    max_concurrent_runs: 2

This is because the execution manager requires the instance itself, so cannot be a fully hydrated
configurable class

alangenfeld accepted this revision.Thu, Jan 9, 10:50 PM

Update [1] then good to go I think

python_modules/dagster/dagster/core/instance/__init__.py
213–242

[1]

This revision is now accepted and ready to land.Thu, Jan 9, 10:50 PM
prha updated this revision to Diff 8574.Fri, Jan 10, 12:20 AM

add dagit settings info to dagit info panel

prha updated this revision to Diff 8576.Fri, Jan 10, 12:28 AM

rebase