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

Workflow Invocation view improvements #18615

Merged
merged 24 commits into from
Nov 12, 2024

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Jul 29, 2024

Following discussions at GCC 2024 and this roadmap by @nekrut: https://hackmd.io/@nekrut/HkinoEh8A

Overall Change to the component

Before After
invocation_view_improvements_before invocation_view_improvements_after
Steps show on the right, the progress bars are within the Overview tab Only the currently active step shows below, leaving full width for graph, progress bars in the Header

Changes to an actively running workflow

Changes made to WorkflowRunSuccess:

Before
workflow_run_success_before
After
workflow_run_success_after

Other Changes

Changes from the roadmap and other changes that improve the UX:

- Steps moved to a separate tab, show clicked/active step below instead of job

This removes the steps from the overview and the graph is filling the center as the default view, and instead of clicking a step and then viewing a job below, the active/clicked step is shown below in its entirety:

invocation_graph_steps_removed.mp4

- Improved Workflow Invocation Step component

Before After
step_before step_after

- Inputs shown in Graph

Before After
inputs_before inputs_after
inputs_param_before inputs_param_after

- Invocation Panel toggles out on mouseleave

The first commit tackles point A in the roadmap: List of invocations is now not visible continously, it goes away after you settle on one invocation and mouseleave the panel area:
invocation_panel_toggled_mouseleave

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@ahmedhamidawan ahmedhamidawan changed the title [WIP]: Workflow Invocation view improvements Workflow Invocation view improvements Sep 20, 2024
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review September 20, 2024 23:07
@github-actions github-actions bot added this to the 24.2 milestone Sep 20, 2024
@mvdbeek mvdbeek self-requested a review October 1, 2024 14:43
@mvdbeek
Copy link
Member

mvdbeek commented Oct 1, 2024

The client unit and selenium tests failures look related ?

@ahmedhamidawan ahmedhamidawan force-pushed the invocation_view_improvements branch 2 times, most recently from 9367b99 to 1962645 Compare October 1, 2024 23:03
@mvdbeek
Copy link
Member

mvdbeek commented Nov 5, 2024

We walked through this in the last IWC and it really feels great! I guess one thing that's kind of odd is that a random-ish step is auto-expanded ? Could we make it so that a step needs to be selected in order to be expanded ?

@ahmedhamidawan
Copy link
Member Author

Thank you @mvdbeek and everyone who had a look (sorry I missed the message in the iwc chat for that meeting... 😞)

The random step being highlighted was done in the initial implementation when you had steps next to the graph, and it would (for terminal invocations) when you landed on the view, highlight the first failed step (which was requested).
But maybe now it makes sense to only highlight a step when the user clicks on it. I will add this here

The only other reason I've not finished this up entirely is PR 17708 where I was so far, rebasing and adjusting against dev, and planning to get that in first. But now I realize that I'll be in another painful rebase if that gets merged first?

So, I'll just get this in before that.

ahmedhamidawan added a commit to ahmedhamidawan/galaxy that referenced this pull request Nov 5, 2024
@ahmedhamidawan
Copy link
Member Author

This is good to be merged!

This first commit tackles point A in the roadmap:

List of invocations is now not visible continously, it goes away after you settle on one invocation and `mouseleave` the panel area.

Roadmap: https://hackmd.io/@nekrut/HkinoEh8A
- Instead of showing steps right next to the invocation graph, moves steps (back) into a separate tab
- To make this work, adds a `graphStepsByStoreId` to the `invocationStore` that helps access the graph steps in multiple components (TODO: improve how this is done)
- Instead of clicking on a node/step in the graph and showing the job for the step that the user clicks, now we show the whole expanded step below
add gap between step header buttons
- The progress bars are in `WorkflowInvocationHeader` now, giving more space to the step header being visible below the graph
- `WorkflowRunSuccess` is a one line header (refactored), and it uses `GridInvocation` to show batch invocations.
For the current history, we show a title saying "This is your current history", therefore, a mock is added here for `historyStore.currentHistoryId` to always return `current-history-id` regardless of which history you mount. This is done so that we can test the case when the history is current, and not.
- Adjust props
- Remove the return to invocation list jest, we will be removing that button soon
<div class="d-flex justify-content-between pb-1 pr-1">
<i>
<FontAwesomeIcon :icon="faRegClock" class="mr-1" />run
<UtcDate :date="props.job.create_time" mode="elapsed" />
Copy link
Member

@mvdbeek mvdbeek Nov 7, 2024

Choose a reason for hiding this comment

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

This isn't the duration you would think it is, it's the entire time from scheduling to last to last update. Can you remove this ? Once the job is terminal you could get this from the wallclock job metric, but I would include that directly in the job info page.

Copy link
Member Author

@ahmedhamidawan ahmedhamidawan Nov 7, 2024

Choose a reason for hiding this comment

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

So, this is the create_time; I thought it is the time that the job was created?
In this <i> we show Job was run 18 minutes ago (e.g.)

Are you still talking about this being the entire time from scheduling to last to last update?

Isn't that the time in the next <i>, conditionally rendered for v-if="!NON_TERMINAL_STATES.includes(props.job.state)"?

which itself is calculated as galaxy/client/src/components/JobInformation/utilities.ts:

import { formatDuration, intervalToDuration } from "date-fns";
import type { JobBaseModel } from "@/api/jobs";
export function getJobDuration(job: JobBaseModel): string {
return formatDuration(intervalToDuration({ start: new Date(job.create_time), end: new Date(job.update_time) }));
}
?

For that one, I can look at the wallclock job metric instead?

Here are the two different times shown here:
image

Copy link
Member

@mvdbeek mvdbeek Nov 8, 2024

Choose a reason for hiding this comment

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

This is when the job was scheduled and first written to the database, that has nothing to do with what you'd call duration in a realistic production setup. duration would be wallclock time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see.
So should we remove either of these times from here?
If not, I understand the (terminal) one on the right is incorrect, and so do we still keep the one on the left?
And do we then place the job metric wallclock time here?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I've removed both times from this view, and instead just simply used the JobDetails component that in turn uses the existing JobMetrics component in this latest commit 0c50efc

Screen.Recording.2024-11-11.at.4.33.15.PM.mov

Let me know if this works, since this is just a reuse of the existing job details view we see when we click on a ContentItem's info option.

No need for the `WorkflowInvocationJob` component added in this PR because `JobDetails` has all we need.
@mvdbeek mvdbeek merged commit 723004e into galaxyproject:dev Nov 12, 2024
55 of 56 checks passed
@ahmedhamidawan ahmedhamidawan deleted the invocation_view_improvements branch November 18, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants