Page MenuHomePhabricator

Store executionMetadata tags on PipelineRunData
AbandonedPublic

Authored by sashank on Thu, Sep 5, 5:15 PM.

Details

Reviewers
alangenfeld
schrockn
Group Reviewers
Restricted Project
Summary

Adds a generic tags attribute on PipelineRunData, which stores the tags set on executionParams.executionMetadata.tags. Also stores a dagster/scheduleId tag on pipeline runs that were initiated by a schedule.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
pipeline-run-tags
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 edited the summary of this revision. (Show Details)Thu, Sep 5, 5:17 PM
sashank updated this revision to Diff 4278.Thu, Sep 5, 5:19 PM

Update cron shell script

sashank updated this revision to Diff 4279.Thu, Sep 5, 5:27 PM

rebase + make graphql

sashank updated this revision to Diff 4280.Thu, Sep 5, 5:28 PM

Formatting

alangenfeld accepted this revision.Fri, Sep 6, 6:04 PM
alangenfeld added a subscriber: alangenfeld.

do we enforce Dict[string, sting] for the tags dict earlier up in the APIs?

python_modules/dagster/dagster/core/storage/pipeline_run.py
73

use check

This revision is now accepted and ready to land.Fri, Sep 6, 6:04 PM
sashank updated this revision to Diff 4339.Fri, Sep 6, 7:06 PM

Add check

We do in the ExecutionMetadata constructor, but added it here as well

sashank updated this revision to Diff 4340.Fri, Sep 6, 7:09 PM

change to opt_dict_param

Harbormaster failed remote builds in B3476: Diff 4342!
sashank planned changes to this revision.Fri, Sep 6, 10:47 PM
sashank updated this revision to Diff 4463.Mon, Sep 9, 5:53 PM

Create PlannedPipelineRun

This revision is now accepted and ready to land.Mon, Sep 9, 5:53 PM
sashank requested review of this revision.Mon, Sep 9, 9:21 PM
sashank updated this revision to Diff 4480.Mon, Sep 9, 9:38 PM

Test for tags

sashank updated this revision to Diff 4481.Mon, Sep 9, 9:47 PM

Update SqliteRunStorage

sashank added inline comments.Mon, Sep 9, 9:53 PM
python_modules/dagster/dagster/core/storage/pipeline_run.py
105

open to better naming suggestions for these helper methods

sashank updated this revision to Diff 4483.Mon, Sep 9, 9:56 PM

make graphql

alangenfeld added inline comments.Mon, Sep 9, 10:09 PM
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
30–32

we should probably do a shared interface here

32

StartedPipelineRun
InvokedPipelineRun
NewPipelineRun

just throwing ideas out there since it feels a bit off

46–47

assert status probably

python_modules/dagster/dagster/core/storage/runs.py
164 ↗(On Diff #4483)

this comment doesnt make any sense - think i forgot to remove it

alangenfeld added inline comments.Mon, Sep 9, 10:11 PM
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
30–32

interface
PipelineRun

implementors
CompletedPipelineRun
ActivePipelineRun
PlannedPipelineRun

or we could align to the status enum - but i think failure and success probably have the same fields - not sure if active has anything different though conceptually you should only be able to subscribe on active

alangenfeld added inline comments.
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
30–32

@bengotow you have any thoughts on what you think would make the most sense from the clients perspective?

sashank updated this revision to Diff 4507.Tue, Sep 10, 12:12 AM
  • Use shared interface PipelineRunInterface
  • Assert status of PlannedPipelineRun

After looking at this I'm still very unclear as to why we need a new "Planned" pipeline run graphql type. Can it not just be a different state?

sashank added inline comments.Tue, Sep 10, 5:38 PM
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
30–32

If this implementation looks good, I can rename the interface and implementors as suggested, since a rename of PipelineRun would require a lot of changes throughout the codebase

After looking at this I'm still very unclear as to why we need a new "Planned" pipeline run graphql type. Can it not just be a different state?

The main idea is communicating in the GraphQL schema what information you have access to from a "start pipeline" mutation ie results are not present yet. Internally PipelineRun is a tagged type but this change would make the GraphQL type be separate types with a common interface.

@sashank lets split out the GraphQL schema change in to its own diff since its worth more discussion and shouldnt block the tags storage change

alangenfeld added inline comments.Tue, Sep 10, 6:18 PM
python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
177–190

have you looked in to moving this callsite such that we dont need to do create_or_update?

sashank updated this revision to Diff 4573.Tue, Sep 10, 10:35 PM

Remove PlannedPipelineRun and remove pipline run interface

sashank planned changes to this revision.Tue, Sep 10, 10:35 PM
sashank updated this revision to Diff 4576.Tue, Sep 10, 10:41 PM

make graphql

sashank updated this revision to Diff 4578.Tue, Sep 10, 10:47 PM

add tags resolver

sashank updated this revision to Diff 4581.Tue, Sep 10, 11:05 PM

update mocks

sashank updated this revision to Diff 4586.Wed, Sep 11, 12:56 AM

Remove COMPUTE_LOGS mock

sashank updated this revision to Diff 4590.Wed, Sep 11, 1:35 AM

make graphql

schrockn requested changes to this revision.Wed, Sep 11, 2:21 AM
schrockn added inline comments.
python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
177

I don't understand why this is necessary or required. if we are "updating" a run to be "not started" what does that mean?

python_modules/dagster/dagster/core/storage/pipeline_run.py
90

i think dagster.utils.merge_dict would be more elegant here

105

why is this not just a run_id check? all the other properties you are checking are immutable w/r/t run_id

This revision now requires changes to proceed.Wed, Sep 11, 2:21 AM
sashank planned changes to this revision.Wed, Sep 11, 5:55 PM

Going to remove all non-tag related changes

sashank abandoned this revision.Thu, Sep 12, 6:00 PM