Page MenuHomePhabricator

#2320 add root_run_id to group related runs
ClosedPublic

Authored by yuhan on Mar 25 2020, 5:38 AM.

Details

Summary

Issue: https://github.com/dagster-io/dagster/issues/2320

root_run_id will be created when users hit re-execution on either Run Table or Run.
If it's a new run without children, it won't have this tag, because the current logic is to create the tag in re-execution func.

cleanup

  • refactored getExecutionVariables in Run and RunTable
  • consolidated names of the buttons that are using RunActionButtons
Test Plan
  • dagit
  • BK

Diff Detail

Repository
R1 dagster
Branch
yuhan/run-tag-rep
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

yuhan created this revision.Mar 25 2020, 5:38 AM
yuhan edited the summary of this revision. (Show Details)Mar 25 2020, 5:44 AM
yuhan edited the test plan for this revision. (Show Details)
yuhan added reviewers: max, alangenfeld.

spitballing names
dagster/origin_id
dagster/root_id
dagster/root_run_id
dagster/run_group

though dagster/run_grouping_id isn't too bad

js_modules/dagit/src/runs/Run.tsx
165–183

probably worth refactoring this out to a single function at this point

yuhan updated this revision to Diff 11005.Mar 25 2020, 11:18 PM

refactor getExecutionVariables for Run and RunTable

yuhan updated this revision to Diff 11009.Mar 26 2020, 12:05 AM

consolidate names of buttons using RunActionButtons

yuhan retitled this revision from #2320 add run_grouping_id (root run id) to group related runs to #2320 add root_run_id (root run id) to group related runs.Mar 26 2020, 12:07 AM
yuhan edited the summary of this revision. (Show Details)
yuhan added a reviewer: prha.
yuhan retitled this revision from #2320 add root_run_id (root run id) to group related runs to #2320 add root_run_id to group related runs.
yuhan edited the summary of this revision. (Show Details)
prha added inline comments.Mar 26 2020, 4:39 AM
js_modules/dagit/src/runs/RunActionButtons.tsx
35–41 ↗(On Diff #11011)

nice!

45 ↗(On Diff #11011)

Do we need to truncate? Is it just an HTML title (tooltip)?

If not, is it better to use the ellipsis here in JavaScript or use CSS? (e.g. have you consider setting it to have a max-width and text-overflow?

js_modules/dagit/src/runs/RunUtils.tsx
131 ↗(On Diff #11011)

I find these function names a bit confusing. ReExe is not an abbreviation that jumps to the forefront of the mind. Also, I find it to be a bit of red flag that the names describe what they're used for instead of what the function does.

I have a suspicion also that they could be combined.... into a single getReexecutionVariables (or something similar) function.

yuhan added inline comments.Mar 26 2020, 7:51 PM
js_modules/dagit/src/runs/RunActionButtons.tsx
45 ↗(On Diff #11011)

text-overflow: ellipsis seems not work well with flex box. So either we give up the vertical centered align or the ...

To get both, we will have to manually add ... here as it is now. I would prefer to keep it this way. I tried to use ellipsis, but the overflow stuff made the icon css a bit off because the button and icon margins in bp3. So I had to do extra hack to fix this


plus we don't display the full text in hover anyways (we show the button description instead)

yuhan marked 3 inline comments as done.Mar 26 2020, 10:50 PM
yuhan added inline comments.
js_modules/dagit/src/execute/ExecutionStartButton.tsx
162–164 ↗(On Diff #11029)

js_modules/dagit/src/runs/RunActionButtons.tsx
45 ↗(On Diff #11011)

nvm... found a way to fix it^
see the comment in ExecutionStartButton.tsx

yuhan updated this revision to Diff 11030.Mar 26 2020, 10:57 PM

add a comment

yuhan added inline comments.Mar 26 2020, 11:00 PM
js_modules/dagit/src/runs/RunTable.tsx
55

cc @alangenfeld I was about to refactor this back, but found D2005. So Im adding a comment here for future reference

alangenfeld added inline comments.Mar 27 2020, 4:32 PM
js_modules/dagit/src/runs/RunTable.tsx
55

<3

alangenfeld accepted this revision.Mar 27 2020, 4:47 PM
alangenfeld added inline comments.
js_modules/dagit/Makefile
16–17 ↗(On Diff #11030)

am i correct this updates the behavior from fixing to listing? we sure we want to do that?

This revision is now accepted and ready to land.Mar 27 2020, 4:47 PM
max accepted this revision.Mar 27 2020, 5:53 PM
max added inline comments.
js_modules/dagit/src/execute/ExecutionStartButton.tsx
162–164 ↗(On Diff #11029)

i suspect as we expand this to allow subset re-execution that we will probably end up with "Re-execute single step" "Re-execute subset"

js_modules/dagit/src/runs/RunActionButtons.tsx
41 ↗(On Diff #11030)

all our sins in one place

js_modules/dagit/src/runs/RunUtils.tsx
80 ↗(On Diff #11030)

would love to eventually get these magic string keys factored out too

yuhan added inline comments.Mar 27 2020, 7:50 PM
js_modules/dagit/Makefile
16–17 ↗(On Diff #11030)

It throws errors now because we don't give any args. I'm gonna switch to yarn run prettier --write "./src/**/*.tsx"

This revision was automatically updated to reflect the committed changes.