Page MenuHomePhabricator

Create Scheduler UI in Dagit
ClosedPublic

Authored by sashank on Aug 21 2019, 4:23 PM.

Details

Reviewers
bengotow
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:5e16ff8a6911: Create Scheduler UI in Dagit
Summary

This diff introduces a UI for the scheduler. It is hidden behind the scheduler feature flag. It displays the list of schedule definitions and a list of currently running schedules. The running schedule rows are pretty empty for now, but we can use the space to add operational info (such as past runs, whether they succeeded, etc.).

Test Plan
  1. Set scheduler feature flag in $DAGSTER_HOME/dagster.cfg:
[FEATURES]
scheduler=

Or simply:

echo "[FEATURES]\nscheduler=" > $DAGSTER_HOME/dagster.cfg
  1. Create a repository with a scheduler:
# repository.yaml

repository:
  file: repo.py
  fn: define_repo
# repo.py

from dagster import solid, pipeline, RepositoryDefinition, ScheduleDefinition
from dagster_cron import SystemCronScheduler


@solid
def hello_world(_):
    return "Hello world"


@pipeline
def hello_world_pipeline():
    return hello_world()


def define_repo():
    hello_world_every_minute_schedule = ScheduleDefinition(
        "hello_world_every_minute",
        cron_schedule="* * * * *",
        execution_params={
            "environmentConfigData": {"storage": {"filesystem": None}},
            "selector": {"name": "hello_world_pipeline", "solidSubset": None},
            "mode": "default",
        },
    )

    return RepositoryDefinition(
        name="demo_repository",
        pipeline_defs=[hello_world_pipeline],
        experimental={
            'scheduler': SystemCronScheduler,
            'schedule_defs': [hello_world_every_minute_schedule],
        },
    )
  1. Run dagster schedules start hello_world_every_minute
  1. Run dagit -p 3333

Diff Detail

Repository
R1 dagster
Branch
schedule-ui
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sashank updated this revision to Diff 4131.Fri, Aug 30, 12:17 AM
sashank edited the summary of this revision. (Show Details)

Clean up

sashank updated this revision to Diff 4132.Fri, Aug 30, 5:33 PM

Update tests

sashank edited the summary of this revision. (Show Details)Fri, Aug 30, 9:27 PM
sashank edited the summary of this revision. (Show Details)Fri, Aug 30, 9:49 PM
sashank edited the summary of this revision. (Show Details)
sashank updated this revision to Diff 4149.Fri, Aug 30, 10:43 PM
sashank edited the summary of this revision. (Show Details)

Extract shared components between SchedulesRoot and RunHistory

sashank retitled this revision from [WIP] Create scaffolding for dagit scheduler UI to Create scaffolding for dagit scheduler UI.Fri, Aug 30, 10:44 PM
sashank added a reviewer: Restricted Project.
sashank edited the summary of this revision. (Show Details)
sashank retitled this revision from Create scaffolding for dagit scheduler UI to Create Scheduler UI in Dagit.
prha added a subscriber: prha.Tue, Sep 3, 4:31 PM

In my patch of this diff, I keep getting GraphQL errors because the scheduler is not defined on the context. Do I need to have some experimental flag on for my repo definition?

I saw https://github.com/dagster-io/dagster/issues/1693, but we probably should handle this differently... (e.g. don't show the link if it's going to error out anyway)

js_modules/dagit/src/schedules/SchedulesRoot.tsx
34

do we expect to handle lots of different scheduler types in the future? We could switch to a ternary or something...

42

This helper method is not doing very much... consider inlining?

TEST PLAN
manual

good to be explicit in the manual case since its only reproducible if the steps you took are known, doesn't need to be crazy thorough but I should at least be able to know how/if you tested with the scheduler feature flag on and off for example

sashank planned changes to this revision.Tue, Sep 3, 4:53 PM
sashank edited the test plan for this revision. (Show Details)Wed, Sep 4, 6:02 PM
sashank edited the test plan for this revision. (Show Details)
sashank edited the test plan for this revision. (Show Details)
sashank updated this revision to Diff 4232.Wed, Sep 4, 6:03 PM
sashank marked 2 inline comments as done.
sashank edited the test plan for this revision. (Show Details)

Inline render methods

sashank planned changes to this revision.Wed, Sep 4, 6:03 PM
sashank edited the test plan for this revision. (Show Details)Wed, Sep 4, 8:30 PM
sashank edited the test plan for this revision. (Show Details)Wed, Sep 4, 9:21 PM
sashank updated this revision to Diff 4240.Wed, Sep 4, 9:22 PM

Handle case where scheduler is not defined and capture graphql error

schrockn requested changes to this revision.Thu, Sep 5, 1:16 PM
schrockn added a subscriber: schrockn.

Not looking at the code, but just clicking around.

  • Looks like the link to look at a pipeline doesn't work. The link should go to http://localhost:3000/p/hello_world_pipeline/explore not `http://localhost:3000/explore/hello_world_pipeline
  • I don't understand the language "Running Schedules" versus "Schedule Definitions". You cannot have multiple instances of a definition, correct? Seems like "Inactive Schedules" versus "Active Schedules" or something would be good. I would in fact just have a single grid of "Schedules" with an active/inactive button that is clickable. We can have it sorted by "Active/Inactive" and then "Schedule Name" by default.
  • In order for the web server to pick up the changes in schedule, I have to restart it.
This revision now requires changes to proceed.Thu, Sep 5, 1:16 PM
bengotow added inline comments.Thu, Sep 5, 2:52 PM
js_modules/dagit/src/App.tsx
15

This is a really stupid nit but it'd be nice if the filename and the exported component name where the same

js_modules/dagit/src/TopNav.tsx
18

Just out of curiosity, is there a pattern to this rearrangement? Would be happy to adopt a convention (I think I typically do built-ins / external packages in a block and project imports in another block), but I can't quite tell what changed here.

js_modules/dagit/src/runs/RunHistory.tsx
314 ↗(On Diff #4243)

Just to make sure, this is going back to /runs/:runId? This looks slightly merge conflicty.

js_modules/dagit/src/schedules/SchedulesRoot.tsx
54–58

I know this isn't super helpful feedback, but it's a little confusing that there are schedules and also scheduler.schedules which have a different set of attributes. I wonder if we could come up with another term for one of these? crontabEntries for the first set maybe?

sashank updated this revision to Diff 4287.Thu, Sep 5, 8:40 PM
sashank marked an inline comment as done.
  • Rename SchedulerRoot to SchedulesRoot
  • Fix incorrect links due to bad merge
  • Rename scheduler.schedules to schedules.runningSchedule
sashank planned changes to this revision.Thu, Sep 5, 8:40 PM
sashank added inline comments.
js_modules/dagit/src/TopNav.tsx
18

Ah this wasn't intentional. My editor is doing this automatically – looks like it's using the sort-imports rule from ESLint: https://eslint.org/docs/2.0.0/rules/sort-imports. Does VSCode not automatically format it this way for you?

It goes:

  • none - import module without exported bindings.
  • all - import all members provided by exported bindings.
  • multiple - import multiple members.
  • single - import single member.

And then alphabetically within those four groups.

js_modules/dagit/src/runs/RunHistory.tsx
314 ↗(On Diff #4243)

Good catch - this was a bad merge

js_modules/dagit/src/schedules/SchedulesRoot.tsx
54–58

This is definitely confusing – I renamed scheduler.schedules to scheduler.runningSchedules

sashank updated this revision to Diff 4288.Thu, Sep 5, 8:42 PM

Fix link to pipeline

sashank updated this revision to Diff 4290.Thu, Sep 5, 9:02 PM

Fix tests

sashank planned changes to this revision.EditedThu, Sep 5, 11:44 PM

Update on the UI:

When hovering cron schedule:

updated UI looks fantastic

sashank updated this revision to Diff 4536.Tue, Sep 10, 6:46 PM

Remove latest runs (will add in upcoming diff)

sashank edited the summary of this revision. (Show Details)Tue, Sep 10, 6:49 PM
sashank updated this revision to Diff 4539.Tue, Sep 10, 6:54 PM

make graphql

alangenfeld accepted this revision.Tue, Sep 10, 9:26 PM
sashank updated this revision to Diff 4568.Tue, Sep 10, 9:59 PM

make graphql

sashank updated this revision to Diff 4569.Tue, Sep 10, 10:08 PM

Update mock

This revision is now accepted and ready to land.Tue, Sep 10, 10:44 PM
sashank updated this revision to Diff 4591.Wed, Sep 11, 1:47 AM

update snapshots

This revision was automatically updated to reflect the committed changes.