Fix Pipeline Job titles are partially handled #1517
Conversation
@andrewballantyne I am sorry, I just realized I misunderstood the issue and it's a implementation mistake, hence adding label to WIP. |
There was a problem hiding this comment.
@uidoyen Okay, sorry about the delay -- I finally got time to look this up and do a proper deep dive back on my original (last min) implementation that has left a lot desiring (hence the issue).
Let me explain the background, the "what" I did, and the place I think we need to get to (the last one is definitely something you can help us define).
Background:
- When we originally did the mocks and designed this feature, we were under the impression "Scheduled" runs were just runs with extra information about "rerunning"
- When implementation got to the plate, we realized there is this thing called Jobs. These Jobs are actually the "Scheduled" runs -- which in turn are not actually runs, but "things" that create runs
- When we started development and almost all the way to the end of the cycles, we didn't have Jobs working from the backend -- so we didn't see the UX issue until very late in the cycle
- DS Pipelines resources have something called
resource_references-- this is an array of connections- Almost all resources have at least one of these (Experiments might not 🤔)
- Pipelines often have an experiment reference (not sure if "Default" gets auto added if not specified)
- Immediately executed runs often have a pipeline and experiment reference (I'm not sure how pure this is, if execution happens elsewhere outside of our UI, it might do something else)
- Scheduled runs have a JOB and experiment reference -- this is where it differs and gets into the problematic space
- if your run does not have a Pipeline reference, how do we know what Pipeline it was executed against?
- Essentially this is done by going through the Job; Scheduled Run => Job => Pipeline (this is done through the
resource_referenceon each resource)
- Essentially this is all to say, without proper effort, we ended up getting something like
testkdswp-15-2214404825being created as a run as the Job executed- This looked out of place and I brought this up to UX in our final review just before slapping the release sticker on the code (this predates incubation)
- This also had no "Pipeline" reference because the run only pointed out it was executed from a Job
The last min fix:
- Having now seen how it looked -- odd name, no pipeline -- we realized it was a notable drop in usefulness to the user to see it without us trying to do something about it
- What I did was I walked to the Job resource (how I did it in code) as I needed it and acted on that resource set of information.
- This was designed as a lazy-load mechanism and heavily relies on the setState re-triggering the render so the function can provide valuable info... we may want to change this to a rendering of the value as a ReactNode instead of as pure data (to make the rendering the same)
- Problem is, I was narrow with my solution -- I did Pipeline Table rendering, as that's what we saw immediately during UX review
- Delete Modals, Searching, Details Pages, none of these got the same treatment and thus show the old name
The place we need to get to:
- A unified UI rendering experience
- Immediately executed runs still exist, they need to still render the desired way
- Scheduled runs will be intermixed, they need to render nicer in all places they are rendered
- They are interchangeable -- in all places both scenarios are in play, it can either be a Job's run, or a pure run
- An easy code-level way of getting this information in a way that is useful / helpful (maybe we keep my hook, maybe we toss it for something better)
- Consider we might want to use a Component to just render the data and it can figure out which state to render in
- Low churn per use-case, an implementation needs to support the places where we need to fix this
- Global Runs => Triggered Tab => Table row rendering
- Global Runs => Triggered Tab => Table filtering
- Global Runs => Triggered Tab => Delete modal; for delete one (row kebab) & delete many (checkboxes + the toolbar kebab)
- Pipeline Run Details Page (this should be linkable from all 3 locationsA) -- the name in the title needs to
- I believe even duplicate run might use the old name and not the display name we chose to render -- you'll have to look into this as well
- Whatever our solution is, we need to make sure we configure the data once and then relay that information purely on rendering or in filters
Notes:
- A - There are 3 locations in Pipelines. The Project Details, the Global Pipelines, and the Global Run pages. These three locations support a lot of the same pages (this is most notable in breadcrumbs, but also are supported by different routes). This is not perfect and there are gaps in some of the implementations around this
- It is worth noting, a scheduled run does not directly associate to a Pipeline, and thus never shows up in the Pipeline views -- since the Run is part of a Job. This is out of scope for this issue, but it is worth noting.
I get this is a lot to take in and not great to do this as a PR review. Let us talk more if you want, but take the time to absorb this information and understand the problem case more. Sorry to have made the ticket vague -- I expected to be the one to do this so my thoughts didn't really need to be documented per se.
uidoyen
force-pushed
the
odh-1242
branch
2 times, most recently
from
f8d22d8
to
8ac696d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold Upcoming design problem with moving to API driven functionality. I started a slack thread (internal) with UX. I inquired from the Pipeline team (public) about possibly improving the Run names from Jobs. |
Runs - Recurring and Immediate Triggered Run
Recurring Triggered Run details
Immediate Triggered Run details
Delete single Triggered run
Delete multiple Triggered runs
|
|
@kywalker-rh @kaedward @yannnz For now, I have used |
There was a problem hiding this comment.
I have two issues, a part from the suggested changes proposed here https://docs.google.com/document/d/1moQoJNWCYhg_c9DjcdriM62gP2IJWSj623J6cMsIQJw/edit?usp=sharing
frontend/src/concepts/pipelines/content/DeletePipelineCoreResourceModal.tsx
Outdated
Show resolved
Hide resolved
uidoyen
force-pushed
the
odh-1242
branch
2 times, most recently
from
8c116e4
to
2eb91c4
Compare
|
|
I dont know whether this is related to this particular issue , But when we duplicate the recurring run the pipeline is not prefilled in the duplicate form ... |
mmm i think we addressed that recently, hold on. |
There was a problem hiding this comment.
- Able to see
RecurringandOne-offlabels - Description is reflected properly in both immediate and recurring runs on their respective pages, including the tables.
- Deleting single and multiple runs works fine.
- One concern(this may not be related): Even if you
Disablethe run should we still be seeingOn-goingin theScheduledrow? Shouldn't that either bestoppedorDisabled?
@manaswinidas regarding your comment we already have status enabled/disabled button to indicate the status 
|
There was a problem hiding this comment.
Some more changes needed. Looking really good though. We need to handle the network thing before we can consider merging this.
I'm thinking we may also want to take this to incubation first -- this seems like a big enough footprint. Maybe merge this into f/pipeline-enhancements instead of main?
| deleteName={ | ||
| resourceCount === 1 ? toDeleteResources[0].name : `Delete ${resourceCount} ${type}s` | ||
| } | ||
| > | ||
| {resourceCount <= 1 ? ( | ||
| <>This action cannot be undone.</> | ||
| <> | ||
| The <strong>{toDeleteResources[0]?.name}</strong> run will be deleted.{' '} |
There was a problem hiding this comment.
@lucferbux this is a bug he is introducing, directly related to code he is changing. If it can't happen, it's a minor logic bug; code changes all the time and introducing a bad state is not good.
@uidoyen You cannot change from a static text to a dynamic value and not handle the dynamic nature of the value. You're not going to run into a runtime error, but there may be a case where it is 0 and now you're in trouble trying to grab the first item The **undefined** run will be deleted is not a good look.
At least handle it somehow... eg:
The{toDeleteResources[0] ? ` <strong>${toDeleteResources[0].name}</strong>` : ''} run will be deleted.{' '}
| 'loading...' | ||
| ) : data ? ( | ||
| <Text component={TextVariants.p} className="pf-u-pb-sm"> | ||
| {`Run ${getPipelineJobExecutionCount(resource.name)} of ${data?.name}`} |
There was a problem hiding this comment.
| {`Run ${getPipelineJobExecutionCount(resource.name)} of ${data?.name}`} | |
| Run {getPipelineJobExecutionCount(resource.name)} of {data?.name} |
Is there a reason to not just. use JSX?
| {jobReference ? ( | ||
| <Label color="blue">{PipelineRunLabels.RECURRING}</Label> | ||
| ) : !pipelineReference ? ( | ||
| <Label color="blue">{PipelineRunLabels.RECURRING}</Label> | ||
| ) : ( | ||
| <Label color="blue">{PipelineRunLabels.ONEOFF}</Label> | ||
| )} |
There was a problem hiding this comment.
I think we need tooltips in here -- it'll help users understand what these mean and why they are there.
Tooltips inline as comments.
{jobReference ? (
// Created by a scheduled run
<Label color="blue">{PipelineRunLabels.RECURRING}</Label>
) : !pipelineReference ? (
// Created by a scheduled run that was deleted
<Label color="blue">{PipelineRunLabels.RECURRING}</Label>
) : (
// Run once immediately after creation
<Label color="blue">{PipelineRunLabels.ONEOFF}</Label>
)}
| run: PipelineRunKF; | ||
| statusIcon?: boolean; | ||
| hasJobReference?: boolean; | ||
| PipelineRunLabel?: boolean; |
There was a problem hiding this comment.
Params are camelCase unless it's a component function prop -- of which that would not be boolean. Boolean values are also often a pseudo-question...
Suggest:
| PipelineRunLabel?: boolean; | |
| includeTypeLabel?: boolean; |
| const { namespace } = usePipelinesAPI(); | ||
| const { getJobInformation } = useJobRelatedInformation(); | ||
| const { data, loading } = getJobInformation(resource); | ||
| const loadingState = <Skeleton />; |
There was a problem hiding this comment.
Is there a reason this is a variable and not just inline?
|
|
||
| const filteredRuns = runs.filter((run) => { | ||
| const runValue = filterData[FilterOptions.NAME]; | ||
| const experimentValue = filterData[FilterOptions.EXPERIMENT]; | ||
| const pipelineValue = filterData[FilterOptions.PIPELINE]; | ||
| const startedValue = filterData[FilterOptions.STARTED]; | ||
| const statusValue = filterData[FilterOptions.STATUS]; | ||
| const { data } = getJobInformation(run); |
There was a problem hiding this comment.
43 requests to the same endpoint:
This is a performance problem -- we need to centralize this information. Maybe we need a Context or something -- maybe adding it to the usePipelineAPI so it can reuse values is what we need to do here.
There was a problem hiding this comment.
Can you fix a bug while you're here...
The delete modal has a weird condition:
<DeletePipelineCoreResourceModal
type="triggered run"
toDeleteResources={deleting && run ? [run] : []}
onClose={() => {
navigate(contextPath ?? `/pipelineRuns/${namespace}`);
}}
/>
Can we just close the modal -- seems odd to cancel the delete and navigate. Must have been a copy/paste logic fault on my part.
| export const getPipelineJobExecutionCount = (resourceName: string) => { | ||
| const regex = /\w+(?=-[^-]*$)/; | ||
| return resourceName?.match(regex)?.[0]; | ||
| }; |
There was a problem hiding this comment.
Please write tests on this -- there is a failure state here you're not watching.
Also type your response from this function.
There was a problem hiding this comment.
getPipelineJobExecutionCount('test')
undefined
getPipelineJobExecutionCount('test-5-something')
'5'
getPipelineJobExecutionCount('test-something')
'test'
There are other use-cases here -- I think you need to take a stance on what this is intended on doing. Because test is not a count.
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabDetails.tsx
Show resolved
Hide resolved
uidoyen
force-pushed
the
odh-1242
branch
2 times, most recently
from
6c6cd45
to
7f8fbe3
Compare
|
@andrewballantyne after moving 
|
There was a problem hiding this comment.
@andrewballantyne after moving
useJobRelatedInformationto the context It always returnsnullI am sure i have done something wrong in the context implementation herefrontend/src/concepts/pipelines/context/PipelinesContext.tsx
@uidoyen This should help solve your issue I imagine
| const getJobInformation: GetJobInformation = () => { | ||
| const jobStatus: JobStatus = { | ||
| loading: true, | ||
| data: null, | ||
| }; | ||
|
|
||
| return jobStatus; | ||
| }; |
There was a problem hiding this comment.
Delete this -- thread the value from:
const {
hasCR,
crInitializing,
serverTimedOut,
apiState,
namespace,
project,
refreshAPIState: refreshAllAPI,
} = React.useContext(PipelinesContext);
So once you delete it -- the value you import will take its place:
const {
...
getJobInformation,
} = React.useContext(PipelinesContext);
And this should make it work.
There was a problem hiding this comment.
@andrewballantyne updated accordingly. Thanks!
andrewballantyne
added
the
pr/no-tests-allowed
uidoyen
force-pushed
the
odh-1242
branch
4 times, most recently
from
bcd0547
to
ab8e630
Compare
There was a problem hiding this comment.
This PR has moved on from the original description (eg outdated images) -- but it also has lingered for a while due to me prioritizing other tasks.
There is a lot of good work here -- I will log a follow up (#1791) to catch the things I think need to still be done. But let us get the last 2 months of work in as a foundation.
Thanks for working with me to get it to a place that I am happy with @uidoyen -- I'll log an issue and point at this review for the items.
| @@ -24,7 +39,7 @@ const DeletePipelineCoreResourceModal: React.FC<DeletePipelineCoreResourceModalP | |||
| const [deleteStatuses, setDeleteStatus] = React.useState<(true | Error | undefined)[]>([]); | |||
| const abortControllerRef = React.useRef(new AbortController()); | |||
| const notification = useNotification(); | |||
|
|
|||
| const [isExpanded, setIsExpanded] = React.useState(true); | |||
There was a problem hiding this comment.
This should reset when the modal is closed.
| <ListItem key={resource.id} icon={icon}> | ||
| <TextContent> | ||
| <Text component={TextVariants.h6}> | ||
| {resource.name}{' '} | ||
| {type === PipelineType.TRIGGERED_RUN && ( | ||
| <PipelineRunTypeLabel resource={resource} /> | ||
| )} | ||
| </Text> | ||
|
|
||
| {type === PipelineType.TRIGGERED_RUN && ( | ||
| <PipelineJobReferenceName resource={resource} /> | ||
| )} | ||
| </TextContent> | ||
| </ListItem> | ||
| ); | ||
| })} | ||
| </List> |
There was a problem hiding this comment.
This needs two things:
Indented items so they appear under the expandable:
The name needs to be converted in the body of the message for the simple use-case:
cc @yannnz
| name: initialData?.name ? `Duplicate of ${initialData.name}` : '', | ||
| description: pipelineRunJob?.description ?? initialData?.description ?? '', | ||
| }; | ||
| setFunction('nameDesc', nameDesc); |
There was a problem hiding this comment.
I'm still working on this -- we should re-use existing details and only apply our description if somehow the resource doesn't have one. We should not touch the name -- that can be picked up from the earlier flows.
Ashique and I talked about this -- I wasn't able to work out the TS logic in time. We can swing back.
| return useUpdateData( | ||
|
|
||
| const updatedData = useUpdateData( | ||
| updatedSetFunction, | ||
| initialData, | ||
| 'pipeline', | ||
| getPipelineCoreResourcePipelineReference, | ||
| usePipelineById, | ||
| ); | ||
|
|
||
| return updatedData; |
There was a problem hiding this comment.
Unnecessary change -- we shouldn't have modified this -- but it's not the end of the world.
| @@ -160,6 +200,11 @@ export const useUpdateRunType = ( | |||
| }, [setFunction, initialData]); | |||
| }; | |||
|
|
|||
| const getParams = (pipeline?: PipelineKF): RunParam[] | undefined => | |||
There was a problem hiding this comment.
I think this logic is dealt with in other spots -- we may want to look for those and make this a unified utility.
There was a problem hiding this comment.
This file is getting too long for the original use-cases... we should look at breaking it up into multiple files and storing it all under a runFormData function or something.
Makes things easier than skimming over 200+ lines.
| export enum PipelineRunLabels { | ||
| RECURRING = 'Recurring', | ||
| ONEOFF = 'One-off', | ||
| } | ||
| export enum PipelineType { | ||
| SCHEDULED_RUN = 'scheduled run', | ||
| TRIGGERED_RUN = 'triggered run', | ||
| } |
There was a problem hiding this comment.
These can find a better home -- there is a const file we can put them in.
| export const getPipelineJobExecutionCount = (resourceName: string) => { | ||
| const regex = /(\w+)(?:-[^-]*)?$/; | ||
| const match = resourceName?.match(regex); | ||
| return match ? match[1] : null; | ||
| }; |
There was a problem hiding this comment.
This will need tests -- I am still not sure about how well this will work if somehow we feed it accidental non-Job naming.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
merged commit f88283c
into
opendatahub-io:f/pipelines-enhancement
Closes: #1242
Description
#1242
It fixes "When a Pipeline Scheduled Run (aka a Job) creates Triggered Runs it creates it with a rather unpleasant name -- a fully lowercase, concatenated together string of the name of the Job."
Multiple runs delete
Single run delete
Run details - including bottom drawer
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main