-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
add display_name field to graphs endpoint #8287
Conversation
✅ Deploy Preview for prefect-orion ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
bc9b71e
to
88b7d42
Compare
@@ -351,6 +352,7 @@ async def read_task_run_dependencies( | |||
"state": task_run.state, | |||
"expected_start_time": task_run.expected_start_time, | |||
"name": task_run.name, | |||
"display_name": task_run.task_key.rpartition(".")[-1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we partition on a period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if anything, I think the display name should be a portion of the task run name not the task key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task_key
is delimited by a .
, in instances where there is no period, like when a user has defined a key manually, rpartition
will still result in an array where the last value is the whole word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. The task_key
defaults to the qualified import name of the task, a right-partition on a period does make sense here. However, the task key doesn't really make sense as the display name for a task run since task runs won't be distinguishable? A right partition of the task_key
is also the same as the default value for the task name so if you are just trying to show task names you should probably use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. If there are more than one instance of a task run in a flow, it will show up twice with the same name, but the intent is for the UI to surface the unique name on click. The unique string at the end is not very human readable on it's own and clutters charts, making it more harmful than helpful when simply fallowing the series of events.
You can see that demonstrated here:
https://user-images.githubusercontent.com/6776415/215178374-c839420b-5bbd-461b-9501-2a602846e9e9.mov
The individual task run name will be available when exploring the task details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be weird once we start letting users define their own names.
Can you add test coverage for this?
Closing this as we explore ways of making the task_run name itself be somewhat simplified and more future proofed for when the user is able to create their own name values. |
Resolves PrefectHQ/graphs/issues/55
This PR adds a
display_name
field to the graphs endpoint, so that on the Flow Run Timeline, we can show the task name in a more human readable way, without the unique id's that make the name super long:See `display_name` compared to `name`.
This was achieved by grabbing the end of the
task_key
, which consistently uses the original task name (either the default, or user specified one) at the end, according to a scanning of the latest 1000 records in staging.Checklist
<link to issue>
"fix
,feature
,enhancement