Graphene layer for serving the Runs Feed #23375

jamiedemaria · 2024-08-02T14:08:33Z

Summary & Motivation

Introduces a way to query for the Runs Feed (backfills and runs that are not part of backfills).

Main entrypoint is the GrapheneRunsFeedConnection that returns a list of GrapheneRunsFeedEntrys, a cursor, and whether more entries exist for future calls.

GrapheneRun and GraphenePartitionBackfill both implement the GrapheneRunsFeedEntry interface, which should provide all of the attributes necessary to render the Runs Feed UI (@bengotow if you find during implementing the UI that something is missing, lmk)

How I Tested These Changes

new unit tests

jamiedemaria · 2024-08-02T14:08:56Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jamiedemaria and the rest of your teammates on Graphite

github-actions · 2024-08-02T14:12:58Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-gqv3jc47n-elementl.vercel.app
https://jamie-graphene-mega-runs.core-storybook.dagster-docs.io

Built with commit 0517c1a.
This pull request is being automatically deployed with vercel-action

jamiedemaria · 2024-08-06T14:12:28Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+
+    # order runs and backfills by create_time. typically we sort by storage id but that won't work here since
+    # they are different tables
+    all_mega_runs = sorted(


Do we need to worry about spicy window in the runs/bulk action table?

technically the problem can occur but I wouldn't worry about it in this context

jamiedemaria · 2024-08-06T14:14:07Z

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

@@ -120,6 +122,18 @@ class GrapheneBulkActionStatus(graphene.Enum):
    class Meta:
        name = "BulkActionStatus"

+    def to_dagster_run_status(self) -> GrapheneRunStatus:


In a stacked PR I want to explore adding more BulkActionStatuses so that we can map more cleanly to DagsterRunStatuses. For example, the COMPLETED status could mean some jobs failed, so always displaying as SUCCESS could hide issues from the user

jamiedemaria · 2024-08-06T14:15:28Z

python_modules/dagster-graphql/dagster_graphql/schema/pipelines/pipeline.py

+        name = "RunType"
+
+
+class GrapheneMegaRun(graphene.ObjectType):


Naming thread! what do we want to call this?

MegaRun

BackfillOrRun

LeadRun

RunSet

??? I'm not particularly happy with any of these so far

instead of a new concrete type, it may be better to use an interface or a union and return the existing underlying types for run and backfill

https://graphql.com/learn/interfaces-and-unions/

Union is easiest from server but then client has to fork all rendering logic based on type , interface allows for a common set of fields to handle in one place. Talking with @bengotow and looking at what fields are common probably best way to navigate. Can always add new fields to the two concrete types to support the interface as well .

updated to use an interface

jamiedemaria · 2024-08-06T14:20:47Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+
+    instance = graphene_info.context.instance
+
+    run_cursor, backfill_cursor = cursor.split(";") if cursor else (None, None)


cursor format: For querying runs, the cursor is the ID of the last run in the returned list. I started with something similar here where the cursor is the last run id and the last backfill id separated by a ";". The difficulty with that is that users would need to iterate through the returned list to find the last run/backfill. We could add a resolver on GrapheneMegaRuns that returns the cursor for them (i think that's reasonable), but i also want to think about cursor formats that might be easier to generate in the first place.

heres what I did for similar problem recently https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/remote_representation/external.py#L85-L112

the cursor should be opaque to the client, it just gets echoed back to the server to fetch more

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_mega_run.py

alangenfeld

especially if we move away from a new concrete type, we can focus the naming on the capability we are adding more ie:

mergedRuns, GrapheneMergedRunsConnection, GrapheneMergedRunInterface/Union

merged runs
grouped runs
collapsed runs
run feed (run feed entry/unit)

alangenfeld · 2024-08-06T15:12:36Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+
+    # order runs and backfills by create_time. typically we sort by storage id but that won't work here since
+    # they are different tables
+    all_mega_runs = sorted(


technically the problem can occur but I wouldn't worry about it in this context

alangenfeld · 2024-08-06T15:15:14Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+
+    instance = graphene_info.context.instance
+
+    run_cursor, backfill_cursor = cursor.split(";") if cursor else (None, None)


heres what I did for similar problem recently https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/remote_representation/external.py#L85-L112

the cursor should be opaque to the client, it just gets echoed back to the server to fetch more

alangenfeld · 2024-08-06T15:33:52Z

python_modules/dagster-graphql/dagster_graphql/schema/pipelines/pipeline.py

+        name = "RunType"
+
+
+class GrapheneMegaRun(graphene.ObjectType):


instead of a new concrete type, it may be better to use an interface or a union and return the existing underlying types for run and backfill

https://graphql.com/learn/interfaces-and-unions/

alangenfeld · 2024-08-06T15:38:41Z

python_modules/dagster-graphql/dagster_graphql/schema/runs.py

+class GrapheneMegaRuns(graphene.ObjectType):
+    results = non_null_list("dagster_graphql.schema.pipelines.pipeline.GrapheneMegaRun")
+
+    class Meta:
+        name = "MegaRuns"
+
+    def __init__(self, cursor, limit):
+        super().__init__()
+
+        self._cursor = cursor
+        self._limit = limit
+
+    def resolve_results(self, graphene_info: ResolveInfo):
+        return get_mega_runs(graphene_info, self._cursor, self._limit)
+


having an intermediate object here we should model this more as a "connection" than just a plural of results. Basically this just means returning cursor and maybe "has more" here. Also might inform a slightly different name. We are not consistent about this but if you grep for "connection" you can see some examples.

https://graphql.org/learn/pagination/#complete-connection-model

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_mega_run.py

jamiedemaria · 2024-08-08T17:19:30Z

@alangenfeld i like the capability-focused naming option. I like RunsFeed(Connection/Entry/...). Maybe it's a bit too narrow on the use case of displaying the runs feed, but i kind of like how clear and precise it is about its purpose

sryza

i like the capability-focused naming option. I like RunsFeed(Connection/Entry/...). Maybe it's a bit too narrow on the use case of displaying the runs feed, but i kind of like how clear and precise it is about its purpose

This makes a lot of sense to me.

FWIW I had been imagining that "mega run" would be the new name for what used to be called "backfill", and we'd use something like "outer run" to refer to what's called "mega run" in this PR, which is a run that's not a "sub-run". Regular ol' runs don't seem particularly "mega" to me.

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

jamiedemaria · 2024-08-09T15:21:21Z

@alangenfeld @sryza I did a renaming pass to align everything on RunsFeedConnection/Entry and got everything switched over to use the connection model. Also modified the cursor a bit to account for another edge case. I think the sum of all the changes makes this ready for another proper review pass

alangenfeld

nice this is close, worth a roundtrip i think tho

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

alangenfeld · 2024-08-09T16:41:16Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+    while len(backfills) < limit:
+        new_backfills = instance.get_backfills(cursor=cursor, limit=limit)
+        if len(new_backfills) == 0:
+            return backfills
+        cursor = new_backfills[-1].backfill_id
+        backfills.extend(
+            [
+                backfill
+                for backfill in new_backfills
+                if backfill.backfill_timestamp <= created_before
+            ]
+        )


were looping here justto support created before filter?

yep, it's not ideal. there isn't support for filtering backfills on anything other than status. I want to add more filter support for backfills, but didn't want to expand this PR. Planning to stack a branch that adds more filtering capabilities

alangenfeld · 2024-08-09T16:43:06Z

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

+    # they are different tables
+    all_runs = sorted(
+        all_runs,
+        key=lambda x: x.resolve_creationTime(graphene_info),  # ideally could just do .creationTime


maybe add a method on the two python objects we can call instead ?

the two objects being GrapheneRun and GraphenePartitionBackfill or RunRecord and PartitionBackfill?

ah right these are the graphene objects, yea i think it would still feel a bit cleaner to add a @property creation_time to call here and then have their resolve_ methods call in to as well but thats more of a nit pick so leave to your call

other option would be to hold off on converting to the Graphene... objects until the very end and then iterate through the final list of objects to return and do the conversion

and add a shared prop to both RunRecord and PartitionBackfill

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py

alangenfeld · 2024-08-09T16:47:09Z

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

+    creationTime = graphene.NonNull(
+        graphene.Float
+    )  # for RunsFeedEntry interface - dupe of timestamp
+    startTime = graphene.Float()  # for RunsFeedEntry interface - dupe of timestamp


nit: maybe we should consolidate on to timestamp instead since its a float ? I feel like time is a better name for returning like a datetime object

i like that

my thinking with keeping createTime, startTime, endTime was that in the future where everything can be a single run, we won't have these duplicate fields on GrapheneRun

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

alangenfeld · 2024-08-09T16:48:55Z

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

@@ -325,6 +353,11 @@ class Meta:
        graphene.NonNull("dagster_graphql.schema.instigation.GrapheneInstigationEventConnection"),
        cursor=graphene.String(),
    )
+    jobName = graphene.String()  # for RunsFeedEntry interface - dupe of partitionSetName


hmm not sure about this part of the interface

yeah i dont know what to do here either. i was thinking about naming it something more generic like target but then it seems like it should encompass assetSelection and assetCheckSelection too. maybe that would be ok?

in the end the question is how would this field be used on the client - so best path is to get @bengotow 's input either in this initial PR or maybe defer it to a second iteration

alangenfeld · 2024-08-12T15:30:13Z

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

+        """Placeholder for this PR. Will do a more thurough pass to accurately convert backfill status
+        to DagsterRunStatus in a stacked branch.
+        """


probably want to edit comment if this is going to land and not be just PR time placeholder

alangenfeld · 2024-08-12T15:31:26Z

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py

+            return GrapheneRunStatus.FAILURE
+        if self.args[0] == GrapheneBulkActionStatus.CANCELED.value:
+            return GrapheneRunStatus.CANCELED
+        return GrapheneRunStatus.CANCELING


maybe map canceling -> canceling and default to something else

alangenfeld · 2024-08-12T15:34:17Z

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_runs_feed.py

+# CURRENT_TIMESTAMP only has second precision for sqlite, so if we create runs and backfills without any delay
+# the resulting list is a chunk of runs and then a chunk of backfills when ordered by time. Adding a small
+# delay between creating a run and a backfill makes the resulting list more interwoven
+CREATE_DELAY = 0.5


how slow does this make the test? is there a way to create the runs/backfills in a setup function / fixture so we only have to do it once for all the test cases? Should we constify how many things we create?

…ill/run is made between pages

bengotow

Will defer to others on the Python bits but the new schema looks good to me! Just left one inline comment

bengotow · 2024-08-12T18:08:29Z

js_modules/dagster-ui/packages/ui-core/src/graphql/schema.graphql

+  runId: String!
+  runStatus: RunStatus!


This schema looks great but giving backfills a runId feels a bit icky -- I wonder if we could call the common interface's id and status just id and status, or potentially make new entryId and entryStatus?

Alternatively it might be nice to put a Grafene description on these fields to clarify that they're just meant to implement this common interface

id and status are already taken (by the backfill id and bulk action status respectively). My hesitation with adding entryId and entryStatus is that we'd have to add those to GrapheneRun as well. I was hoping to keep GrapheneRun as unchanged as possible so that in the future when we've eliminated the need for backfills entirely, the GrapheneRun doesn't have attributes that are there because of an interface that is no longer necessary

I'll definitely add descriptions though. I'm also open to pushback on not doing entryId/Status. If it adds significant confusion/code smell it might not be worth it

you might be able to get by with id since its String (or ID) in both cases

the status one i think is useful since it flattens to just one enum

@alangenfeld were you thinking we'd switch GraphenRun.id of GraphenePartitionBackfill.id to either string or ID so that the types match?

the status one i think is useful since it flattens to just one enum

can you elaborate on this a bit?

were you thinking we'd switch GraphenRun.id of GraphenePartitionBackfill.id to either string or ID so that the types match?

I assumed they were the same, i think its fine to change backfill.id to ID to align them, pretty sure that a safe change since ID is String

can you elaborate on this a bit?

as opposed to the id change above, we cant change the return type of either existing status field so we need to add a new field to return one enum type on both objects. That assumes that we want that which @bengotow can speak to

vercel · 2024-08-12T18:27:28Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
dagster-docs-next	❌ Failed (Inspect)			Aug 12, 2024 9:38pm

he steup once

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from 0c1dd35 to dbd5e92 Compare August 5, 2024 16:49

jamiedemaria commented Aug 6, 2024

View reviewed changes

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_mega_run.py Outdated Show resolved Hide resolved

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from c6ac4ac to 034c880 Compare August 6, 2024 14:38

jamiedemaria changed the title ~~Graphene "MegaRun"~~ RFC - Graphene "MegaRun" Aug 6, 2024

jamiedemaria marked this pull request as ready for review August 6, 2024 14:41

jamiedemaria requested review from alangenfeld, sryza and bengotow August 6, 2024 14:41

alangenfeld reviewed Aug 6, 2024

View reviewed changes

jamiedemaria force-pushed the jamie/graphene-mega-runs branch 2 times, most recently from 2eee30f to 28b99d0 Compare August 6, 2024 20:56

jamiedemaria requested a review from alangenfeld August 7, 2024 13:38

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from 48aede2 to 60c8119 Compare August 8, 2024 14:16

sryza reviewed Aug 8, 2024

View reviewed changes

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py Outdated Show resolved Hide resolved

jamiedemaria changed the title ~~RFC - Graphene "MegaRun"~~ Graphene layer for serving the Runs Feed Aug 9, 2024

jamiedemaria requested a review from sryza August 9, 2024 15:21

alangenfeld requested changes Aug 9, 2024

View reviewed changes

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from a375d5b to 831dfa0 Compare August 9, 2024 18:33

jamiedemaria mentioned this pull request Aug 9, 2024

create time filtering for bulk actions table #23560

Merged

jamiedemaria requested a review from alangenfeld August 12, 2024 15:26

alangenfeld approved these changes Aug 12, 2024

View reviewed changes

wip

5042f38

jamiedemaria added 13 commits August 12, 2024 13:40

graphene model

2b600a8

clean up

1a1d032

sort that should account for spicy window

e10f9f8

start some tests

8f97a68

use interface instead

9e9ed7d

test passing

0dc2a8a

more tests

a6cc737

formalize the delay between making a run and a backfill in tests

4eea2e2

rename

5203511

cleanup

6bff148

add timestamp filtering to account for edge case when the first backf…

ec70c07

…ill/run is made between pages

pr comments, enforce limit

136ef06

back to createTime, other small ergonomic improvements

55ab112

bengotow approved these changes Aug 12, 2024

View reviewed changes

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from e9bda94 to 55ab112 Compare August 12, 2024 18:27

vercel bot had a problem deploying to Preview August 12, 2024 18:27 Failure

split slow test setup into it'sown class so we can do t

47ddffc

he steup once

vercel bot had a problem deploying to Preview August 12, 2024 21:07 Failure

graphene comments

89a653e

jamiedemaria force-pushed the jamie/graphene-mega-runs branch from 1b3f915 to 89a653e Compare August 12, 2024 21:38

vercel bot had a problem deploying to Preview August 12, 2024 21:38 Failure

use id instead of runId

0517c1a

jamiedemaria merged commit 21a08e6 into master Aug 14, 2024
2 checks passed

jamiedemaria deleted the jamie/graphene-mega-runs branch August 14, 2024 13:53

PedramNavid pushed a commit that referenced this pull request Aug 14, 2024

Graphene layer for serving the Runs Feed (#23375)

a324266

This was referenced Aug 15, 2024

BulkActions filters in GQL layer #23682

Merged

filter by multiple statuses in BulkActionsFilter #23772

Merged

Reorder status and filters params for get_backfills #23773

Merged


		instance = graphene_info.context.instance

		run_cursor, backfill_cursor = cursor.split(";") if cursor else (None, None)

Graphene layer for serving the Runs Feed #23375

Graphene layer for serving the Runs Feed #23375

Conversation

jamiedemaria commented Aug 2, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

jamiedemaria commented Aug 2, 2024 • edited Loading

github-actions bot commented Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiedemaria Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiedemaria commented Aug 8, 2024

sryza left a comment

Choose a reason for hiding this comment

jamiedemaria commented Aug 9, 2024

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengotow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Aug 12, 2024 • edited Loading

jamiedemaria commented Aug 2, 2024 •

edited

Loading

jamiedemaria commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

jamiedemaria Aug 6, 2024 •

edited

Loading

vercel bot commented Aug 12, 2024 •

edited

Loading