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.

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

Rename run group side bar to Re-executions instead of the job name #26359

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Dec 9, 2024

Summary & Motivation

During dogfooding of some changes to retry logic, @anuthebananu pointed out that the title for the lineage of retried runs was a bit confusing. It's currently the name of the job, which for asset jobs leaks the internal __ASSET_JOB name
Screenshot 2024-12-09 at 3 01 30 PM

This PR renames it Re-executions.
Screenshot 2024-12-12 at 11 28 57 AM

How I Tested These Changes

Changelog

Renamed the run lineage sidebar on the Run details page to Re-executions.

Copy link
Contributor Author

jamiedemaria commented Dec 9, 2024

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

Copy link

github-actions bot commented Dec 9, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-pnbev2e3w-elementl.vercel.app
https://jamie-rename-retry-sidebar.core-storybook.dagster-docs.io

Built with commit 824cc28.
This pull request is being automatically deployed with vercel-action

@@ -96,7 +96,7 @@ export const RunGroupPanel = ({
});

return (
<SidebarSection title={runs[0] ? `${runs[0].pipelineName} (${runs.length})` : ''}>
<SidebarSection title={runs[0] ? `Retries (${runs.length - 1})` : ''}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the update to the number that's shown next to the title? It's currently the number of runs in the run group, which contains the original run. Renaming the section to Retries makes me think the number should be the number of retries, not the total number of runs in the group. However, the list of runs in the section would be the whole run group, so maybe it's more confusing to have Retries (n) but then see n+1 runs in the list

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency it might be best to match whatever is in the retries tag - which I believe in this case is n

Copy link
Member

@gibsondan gibsondan Dec 10, 2024

Choose a reason for hiding this comment

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

I think it is weird to show a number and then have the number of things not match that number (the other lists just below it match the count of their description, e.g. Executing), but it is also weird to call it Retries and then have it not match the number of retries.

It's also maybe a little weird to say "Retries" when the run you're looking at is at the bottom of the stack? Since none of them will be retries of this run

is there a more clear way to say 'Run group'?

Copy link
Member

Choose a reason for hiding this comment

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

... should it just say Runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also maybe a little weird to say "Retries" when the run you're looking at is at the bottom of the stack? Since none of them will be retries of this run

that's a good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spitballing:

  • run lineage
  • retry lineage
  • retry group

Copy link
Member

Choose a reason for hiding this comment

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

maybe worth a post in #eng-naming? "Run Lineage" seems ok to me

@jamiedemaria jamiedemaria marked this pull request as ready for review December 9, 2024 20:10
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

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

@OwenKephart's "Re-executions" suggestion seems reasonable to me

@jamiedemaria jamiedemaria force-pushed the jamie/rename-retry-sidebar branch from d20ce6c to 824cc28 Compare December 12, 2024 16:26
@jamiedemaria jamiedemaria changed the title Rename run group side bar to Retries instead of the job name Rename run group side bar to Re-executions instead of the job name Dec 12, 2024
@jamiedemaria jamiedemaria merged commit a1c6d7e into master Dec 12, 2024
2 checks passed
@jamiedemaria jamiedemaria deleted the jamie/rename-retry-sidebar branch December 12, 2024 16:41
pskinnerthyme pushed a commit to pskinnerthyme/dagster that referenced this pull request Dec 16, 2024
…agster-io#26359)

## Summary & Motivation
During dogfooding of some changes to retry logic, @anuthebananu pointed
out that the title for the lineage of retried runs was a bit confusing.
It's currently the name of the job, which for asset jobs leaks the
internal `__ASSET_JOB` name
<img width="630" alt="Screenshot 2024-12-09 at 3 01 30 PM"
src="https://github.com/user-attachments/assets/8f5841b8-9ea7-4a14-93ec-d373dc8d56a8">

This PR renames it `Re-executions`. 
<img width="572" alt="Screenshot 2024-12-12 at 11 28 57 AM"
src="https://github.com/user-attachments/assets/43feabc3-febe-4573-8b33-9299c722bf4e"
/>


## How I Tested These Changes

## Changelog

Renamed the run lineage sidebar on the Run details page to
`Re-executions`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants