Page MenuHomePhabricator

Initial pass at Dagit “Reload” button based on single watch file
ClosedPublic

Authored by bengotow on Sep 23 2019, 2:36 PM.

Details

Summary

https://github.com/dagster-io/dagster/issues/1684

This diff replaces the dagit watch/no-watch behavior with a button
in the UI that can be clicked to relaunch Dagit (and refresh the web UI without
a page reload!)

There are a few things to discuss here:

  • I removed watch/no-watch, but we could also disable --watch by default and leave it as an option.
  • I think the old watch mode may have been partially broken because it watched './', meaning it would only work if you launched dagit from a repo folder. dagit.py doesn't do enough arg inspection to know where it should be watching. (Eg:, if you run dagit -y /Users/bengotow/Work/F376/Projects/dagster/examples/repository.yaml from your Desktop, it's monitoring your Desktop not the repo.)
  • watchmedo only monitors directories, so we create a tmp directory containing a single file to monitor for changes.
  • Some very whacky weakref magic causes dirs created via seven.TemporaryDirectory() to be deleted when watchmedo relaunches the child process. Moving the invocations of this outside of main() fixes it and I'd love to know why...
  • The UI does not show a "unsaved changes" state. I think we can do this, but it'll be a bit complicated and we may decide we don't want it, so I'll explore that in a stacked diff.
Test Plan

Run tests

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

bengotow created this revision.Sep 23 2019, 2:36 PM
bengotow edited the summary of this revision. (Show Details)Sep 23 2019, 2:40 PM
bengotow added inline comments.Sep 23 2019, 2:44 PM
python_modules/dagit/dagit/cli.py
79

This may be a bit of a cop-out - I could have made this an arg to .get similar to watch_external_runs, but watch_external_runs is threaded through a half dozen factory methods and I didn't want to start turning it into an arg pile for info that isn't required during initialization...

bengotow updated this revision to Diff 4950.Sep 23 2019, 3:06 PM

Fix tests, black formatting, snapshots

schrockn requested changes to this revision.Sep 25 2019, 6:11 PM
schrockn added a subscriber: schrockn.

This is so much nicer.

A few comments:

  • The reload button should instigate a refresh of the page itself. Otherwise to get the update this requires a two step process. The reload of the underlying process and a refresh of the page.
  • Can you add this change to release notes?
  • A tool tip text explaining what reload is doing would be nice. Things to cover:
    • This restarts the underlying web-server process.
    • It reloads the code so changes that effect metadata -- dag structure, solid names, type names -- are updated.
    • You do not need to press this in order for the re-execute button to pick up changes in code.
  • When you manually kill the underling process the reload has an active spinner which is confusing. I think we should just grey out the button. This won't end up rendering the spinner when you press the reload button but I think that is ok. This also occurs where there is something like a syntax error. It could be interesting to actually surface these things somehow as toasts by communicating a graphql error. I think this will be straight forward for the PythonError case by using the dauphin error machinery (keyboard interrupt is tougher.

python_modules/dagit/dagit/cli.py
60

indicate what this defaults to?

python_modules/dagster/dagster/core/instance/__init__.py
209 ↗(On Diff #4950)

can you do a check.XXXX for t so we know what it is?

220 ↗(On Diff #4950)

with open(...) as f please

This revision now requires changes to proceed.Sep 25 2019, 6:11 PM

The reload button should instigate a refresh of the page itself. Otherwise to get the update this requires a two step process. The reload of the underlying process and a refresh of the page.

we shouldnt need a full browser refresh - if we can do something clever to drop all known state and refire all graphql queries (rerender from root & drop apollo cache?) it should be a nice fast experience

bengotow planned changes to this revision.Sep 26 2019, 10:37 PM

Hey folks! I pushed up a few small changes to address the inline comments.

The reload button should instigate a refresh of the page itself. Otherwise to get the update this requires a two step process. The reload of the underlying process and a refresh of the page.

This should already be how it works. It's a big magical, but the call to Apollo.resetStore() makes all the GraphQL queries again without reloading the page. I verified this by changing a pipeline and then clicking reload and making sure the new pipeline graph was shown. If there are other changes not appearing let me know.

Can you add this change to release notes?

Sure

A tool tip text explaining what reload is doing would be nice. Things to cover:
    This restarts the underlying web-server process.
    It reloads the code so changes that effect metadata -- dag structure, solid names, type names -- are updated.
    You do not need to press this in order for the re-execute button to pick up changes in code.

Ok, will add a nice JS tooltip to the button (as opposed to an old school aria tooltip that wouldn't hold all this.) Hopefully that helps!

When you manually kill the underling process the reload has an active spinner which is confusing. I think we should just grey out the button. This won't end up rendering the spinner when you press the reload button but I think that is ok. This also occurs where there is something like a syntax error. It could be interesting to actually surface these things somehow as toasts by communicating a graphql error. I think this will be straight forward for the PythonError case by using the dauphin error machinery (keyboard interrupt is tougher.

Ahh interesting, I think this was an oversight on my part. I wrote a small state machine into the button and it should be fairly safe to only show the spinner AFTER you press the button UNTIL the process comes back? As long as the GraphQL service comes back up, errors should already appear in toasts since it re-requests the GraphQL data, but I'll check. I can also just remove the spinner but it's kind of nice...

python_modules/dagit/dagit/cli.py
60

Hmm it defaults to None and is hidden from the UI - let me see if there's a better way to parse an option that is not meant for external use.

python_modules/dagster/dagster/core/instance/__init__.py
209 ↗(On Diff #4950)

Yep will do!

220 ↗(On Diff #4950)

Oh and it ties the close call to the block scope? This is much better

bengotow updated this revision to Diff 5033.Sep 27 2019, 2:15 AM
  • Basic diff feedback
  • Remove the loading spinner
  • Add to changelog
bengotow edited the summary of this revision. (Show Details)Sep 27 2019, 2:57 PM
alangenfeld added inline comments.Sep 27 2019, 4:03 PM
CHANGES.md
10–11

you can move this up to a ## upcoming (0.6.0) section - https://dagster.phacility.com/D959

python_modules/dagit/dagit/cli.py
78–80

as discussed - pass an object next to instance all the way to the GraphQLContext creation

python_modules/dagit/dagit/dagit.py
42–43

I think we can just share the same directory

82

maybe .stamp.txt

alangenfeld requested changes to this revision.Sep 27 2019, 4:03 PM
This revision now requires changes to proceed.Sep 27 2019, 4:03 PM
bengotow updated this revision to Diff 5099.Sep 27 2019, 6:22 PM
  • Address diff feedback
alangenfeld requested changes to this revision.Sep 27 2019, 7:52 PM
alangenfeld added inline comments.
python_modules/dagit/dagit/cli.py
78–80

not resolved

This revision now requires changes to proceed.Sep 27 2019, 7:52 PM
bengotow added inline comments.Sep 27 2019, 9:16 PM
python_modules/dagit/dagit/cli.py
78–80

Ahh sorry—i keep forgetting arc marks as ready for review whenever you push a diff update. i think i was just switching branches to land that front-end fix. Will get this fixed now!

bengotow updated this revision to Diff 5124.Sep 27 2019, 9:22 PM
  • Move reloading into a separate class given to the GraphQL layer

Hey @alangenfeld this should be ready to go now. I did some research and it looks like watchmedo only allows you to watch directories, not single files, which is why the watch dir is a separate temp directory with one file in it. I was afraid that if we watched the same folder that it uses as a fallback for DAGSTER_HOME, it'd be possible for it to hit the "too many files to watch" scenario again? Let me know what you think

alangenfeld accepted this revision.Sep 27 2019, 9:50 PM

sounds good

intense-nod

python_modules/dagit/dagit/cli.py
78–80

DagsterReloader

DagitReloader right? Its not really relevant to anything except dagit.

make sure to debug that chrome issue before merging

schrockn accepted this revision.Sep 27 2019, 9:51 PM
This revision is now accepted and ready to land.Sep 27 2019, 9:51 PM
alangenfeld added inline comments.Sep 27 2019, 9:53 PM
js_modules/dagit/src/ProcessStatus.tsx
21

did we have an impl of the soft reload (blow away cache) wonder if that would work better given chrome issue observed

bengotow updated this revision to Diff 5228.EditedSep 30 2019, 9:54 PM

Replaces the window.reload() at the beginning of the refresh, which only worked when the React app was running on a separate port in dev mode, with a "soft reload" and a toast to let you know when the reload is complete.

bengotow updated this revision to Diff 5263.Oct 1 2019, 4:46 AM

Define a Reloader ABC in dagster core and move the implementation to Dagit, fix tests by making it optional

bengotow updated this revision to Diff 5266.Oct 1 2019, 5:08 AM
  • Make the reloader optional in create_app as well

move the realoader class to dagster-graphql and youre good to go

python_modules/dagster/dagster/core/reloader/__init__.py
6 ↗(On Diff #5266)

this should be in dagster-graphql i think

bengotow updated this revision to Diff 5277.Oct 1 2019, 4:39 PM

Move the reloader interface to dagster-graphql, fix tests again

bengotow updated this revision to Diff 5289.Oct 1 2019, 5:46 PM
  • Add missing type