Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add BulkActionStatuses COMPLETED_SUCCESS and COMPLETED_FAILURE #24365

Merged
merged 3 commits into from
Sep 16, 2024
Merged

add BulkActionStatuses COMPLETED_SUCCESS and COMPLETED_FAILURE #24365

merged 3 commits into from
Sep 16, 2024

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Sep 10, 2024 •
edited
Loading

Summary & Motivation

Adds two new BulkAction statuses COMPLETED_SUCCESS and COMPLETED_FAILURE to expand on the existing COMPLETED status. This allows us to store the more fine grained information of if the backfill completed with some run failures or if the backfill completed with all runs successful.

How I Tested These Changes

Changelog

Insert changelog entry or "NOCHANGELOG" here.

  • NEW Backfill status will now distinguish between a completed backfill with all runs successful, and completed backfill with some run failures. Previously, these were displayed as the same Completed status. This change only applies to new backfills. Backfills that have already completed will still show a Completed status.

@jamiedemaria Graphite App
Copy link
Contributor Author

jamiedemaria commented Sep 10, 2024 •
edited
Loading

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

Join @jamiedemaria and the rest of your teammates on Graphite Graphite

@jamiedemaria jamiedemaria changed the title add new statuses for backfill completed success and failure add BulkActionStatuses COMPLETED_SUCCESS and COMPLETED_FAILURE Sep 10, 2024
@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 8ede603 to c0f3355 Compare September 11, 2024 16:45
@jamiedemaria jamiedemaria mentioned this pull request Sep 11, 2024
Merged
3 tasks
@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from c0f3355 to ea8ea41 Compare September 11, 2024 19:10
@jamiedemaria jamiedemaria marked this pull request as ready for review September 11, 2024 19:25
@jamiedemaria jamiedemaria requested review from sryza and prha September 11, 2024 19:26
@jamiedemaria
Copy link
Contributor Author

@prha @sryza two discussion topics we can either chat about in this PR or in a meeting this week:

  1. Should the runStatus attr on GrapheneRunsFeedEntry get renamed? It's probably useful to convert to a single type for UI display purposes (but will let @salazarm weigh in on if it's actually that helpful). Some options would be to rename to something like displayStatus, make status a union type and have the UI handle displaying consistent status tags

  2. If we want to do a write-on-read or other migration strategy to move all COMPLETED statuses to one of the new types

I dont think either of these have to block this PR, but want to mark these as topics to discuss at some point

@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from ea8ea41 to 2a8e135 Compare September 11, 2024 19:33
@jamiedemaria jamiedemaria mentioned this pull request Sep 11, 2024
Closed
jamiedemaria
jamiedemaria commented Sep 11, 2024
View reviewed changes
python_modules/dagster-graphql/dagster_graphql/schema/backfill.py
@@ -130,6 +132,10 @@ def to_dagster_run_status(self) -> GrapheneRunStatus:
return GrapheneRunStatus.STARTED # pyright: ignore[reportReturnType]
if self.args[0] == GrapheneBulkActionStatus.COMPLETED.value: # pyright: ignore[reportAttributeAccessIssue]
return GrapheneRunStatus.SUCCESS # pyright: ignore[reportReturnType]
if self.args[0] == GrapheneBulkActionStatus.COMPLETED_SUCCESS.value: # pyright: ignore[reportAttributeAccessIssue]
Copy link
Contributor Author

@jamiedemaria jamiedemaria Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to think of other potential solutions for this conversion (if we maintain the runStatus graphene attr i mentioned in my other comment). It looks like we'll need to convert run statuses to bulk action statuses too for filtering, so i think a consistent solution is in order. Another thing that i can stack on, but isn't strictly required for this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines added here look fine to me. But wondering more generally what the filtering experience looks like -- if I filter to queued runs, do I see runs in the backfill? Or do I see the whole backfill display as a single run if any run is queued? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scheme were going with for filtering is that the row-level object is what gets filtered. So if you are in the combined single runs + backfills view, and filter for queued status, you will only see single-runs that have queued status. and no backfills (even if the backfill has queued runs)

This was referenced Sep 12, 2024
Closed
Merged
@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 2a8e135 to 0923a4b Compare September 12, 2024 16:19
@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 0923a4b to f097359 Compare September 12, 2024 19:44
@jamiedemaria jamiedemaria mentioned this pull request Sep 12, 2024
Merged
clairelin135
clairelin135 reviewed Sep 13, 2024
View reviewed changes
python_modules/dagster/dagster/_core/execution/job_backfill.py Outdated Show resolved Hide resolved
python_modules/dagster-graphql/dagster_graphql/schema/backfill.py
@@ -130,6 +132,10 @@ def to_dagster_run_status(self) -> GrapheneRunStatus:
return GrapheneRunStatus.STARTED # pyright: ignore[reportReturnType]
if self.args[0] == GrapheneBulkActionStatus.COMPLETED.value: # pyright: ignore[reportAttributeAccessIssue]
return GrapheneRunStatus.SUCCESS # pyright: ignore[reportReturnType]
if self.args[0] == GrapheneBulkActionStatus.COMPLETED_SUCCESS.value: # pyright: ignore[reportAttributeAccessIssue]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines added here look fine to me. But wondering more generally what the filtering experience looks like -- if I filter to queued runs, do I see runs in the backfill? Or do I see the whole backfill display as a single run if any run is queued? Or something else?

@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 924c8a3 to 8322a44 Compare September 13, 2024 11:54
@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 8322a44 to 8f9343e Compare September 13, 2024 15:36
@jamiedemaria jamiedemaria mentioned this pull request Sep 13, 2024
Closed
3 tasks
clairelin135
clairelin135 approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@clairelin135 clairelin135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamiedemaria jamiedemaria force-pushed the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch from 8f9343e to 7b447bb Compare September 16, 2024 14:42
@jamiedemaria
display new bulk action statuses in UI (#24409)
26d0664 
## Summary & Motivation
Updates the FE to display the `COMPLETED_SUCCESS` and `COMPLETED_FAILED`
statuses in the downstream PR

## How I Tested These Changes
Backfill run after updating the UI shows `Succeeded` status. Backfill
run before the UI update is still `Completed`. Note that we changed the
language to `Success` instead of `Succeeded` so this screenshot is
slightly out of date

<img width="1576" alt="Screenshot 2024-09-12 at 12 40 50 PM"
src="https://github.com/user-attachments/assets/fbf63a4f-17da-4428-9feb-11c7da7c186c">


## Changelog

Insert changelog entry or "NOCHANGELOG" here.

- [ ] `NEW` _(added new feature or capability)_
- [ ] `BUGFIX` _(fixed a bug)_
- [ ] `DOCS` _(added or updated documentation)_
@jamiedemaria jamiedemaria merged commit 13dbe4f into master Sep 16, 2024
1 of 2 checks passed
@jamiedemaria jamiedemaria deleted the jamie/fou-385-expand-bulkactionstatus-to-encompass-completed-success-and branch September 16, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clairelin135 clairelin135 clairelin135 approved these changes

@sryza sryza Awaiting requested review from sryza

@prha prha Awaiting requested review from prha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jamiedemaria @clairelin135