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

Visualizing workflow runs with an invocation graph view #17413

Merged
merged 42 commits into from
Apr 30, 2024

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Feb 1, 2024

This adds a graph view to the workflow invocation summary, which utilizes the workflow editor canvas and original workflow structure, and instead of showing step details on each step node, it shows the job states for the steps. Clicking on a step expands the details for the invocation step.

The useInvocationGraph composable loads the graph onto the readonly editor based on the original workflow id and invocation object; and the step info is loaded via the step_jobs_summary api route.

An example invocation overview
image

Sample Workflow Run:

invocation_graph_with_run_count.mp4

Invocation View Header:

This also adds a header with details about the invocation:
image

Older (alternative?) Implementation:

With a Card containing everything inline:
image

Hide and Show steps as needed:

To view multiple individual steps at the same time (without the graph getting in the way), users can hide/show steps like so:

hide_and_show_graph.mp4

Invocations List Change: Route to invocation instead of only expanding table rows

This change allows you to click an invocation workflow name and route to the dedicated full page (center-panel) summary for the invocation, while you can still expand it within the table by clicking on the expand details icon in the first column:

Before:

invocations_list_route_to_invocation_before.mp4

Now:

invocations_list_route_to_invocation_after.mp4

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.

@jmchilton
Copy link
Member

I prefer format 1 for what it is worth - cool stuff.

@ahmedhamidawan

This comment was marked as resolved.

@mvdbeek mvdbeek mentioned this pull request Feb 28, 2024
@ahmedhamidawan ahmedhamidawan force-pushed the graph_view_invocations branch 2 times, most recently from 892e214 to a64e496 Compare March 17, 2024 20:29
@ahmedhamidawan ahmedhamidawan changed the title [WIP] Draft of an invocation graph view using the WorkflowGraph Visualizing workflow runs with an invocation graph view Apr 11, 2024
@ahmedhamidawan

This comment was marked as resolved.

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Apr 11, 2024

Backend/API changes:

I needed to make a couple backend changes (commit)

Modify backend schema for InvocationStepJobsResponseJobModel

So that api/invocations/{id}/step_jobs_summary returns job_id in the same form as stored in invocation.steps[n].job_id:

class InvocationStepJobsResponseJobModel(InvocationJobsSummaryBaseModel):
model: JOB_MODEL_CLASS
id: EncodedDatabaseIdField = Field(
default=...,
title="ID",
description="The encoded ID of the job.",
)

Without this change, api/invocations/{id}/step_jobs_summary would return:

[
    {"id":"bbd44e69cb8903b5d7e873ba2496d6ed18a479f3b9f872e5eafc6c082181535e01bd8561292ebce5","states":{"ok":1},"populated_state":"ok","model":"Job"},
    {"id":"bad44e69cb8906b5d7e873da2496d6ed18a479f3b9f872e5f46ef54a25848cfca50b08d03c6f9af3","states":{"ok":1},"populated_state":"ok","model":"Job"}
]

and now it returns:

[
    {"id":"03e4444245c62755","states":{"ok":1},"populated_state":"ok","model":"Job"},
    {"id":"03691da0e97e69c2","states":{"ok":1},"populated_state":"ok","model":"Job"},
]

Add an implicit_collection_jobs_id to InvocationStep summary:

/**
* Implicit Collection Jobs ID
* @description The implicit collection job ID associated with the workflow invocation step.
*/
implicit_collection_jobs_id?: string | null;

because this then helps us store an invocation step's step_jobs_summary in case an invocation step has stepJobSummary.model === "ImplicitCollectionJobs"

@ElectronicBlueberry
Copy link
Member

@ahmedhamidawan I don't think tying the canvas height to the steps is a good idea for several reasons:
The dynamic height of the steps does not play nice with the canvas. This is the issue you noticed. Even if it were to adjust, this would be quite laggy I think, and slow down expanding datasets.

What happens if there are many steps? Does the canvas become very tall? This seems impractical. I'd rather give the canvas a fixed or adjustable height, independent of the steps.

This feature looks very nice and useful btw!
How does this behave with comments?

@ahmedhamidawan
Copy link
Member Author

@ahmedhamidawan I don't think tying the canvas height to the steps is a good idea for several reasons: The dynamic height of the steps does not play nice with the canvas. This is the issue you noticed. Even if it were to adjust, this would be quite laggy I think, and slow down expanding datasets.
What happens if there are many steps? Does the canvas become very tall? This seems impractical. I'd rather give the canvas a fixed or adjustable height, independent of the steps.

The "inner" canvas remains the same height it initially is.
Yes you have a good point of giving it a fixed height and not tying it to the steps. I will try out a way to set a fixed width & height for the two (canvas and steps) to appear side by side (or the steps below in case the screen width is decreased). Thank you!

This feature looks very nice and useful btw! How does this behave with comments?

Thank you @ElectronicBlueberry ! Comments show up exactly as/where they were in the original workflow:

Original Workflow Invocation View
image image

@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review April 16, 2024 19:34
@github-actions github-actions bot added this to the 24.1 milestone Apr 16, 2024
@ahmedhamidawan ahmedhamidawan force-pushed the graph_view_invocations branch 2 times, most recently from 91a8b7b to a601310 Compare April 18, 2024 23:16
@bgruening
Copy link
Member

This is amazing! Thanks a lot @ahmedhamidawan

@ahmedhamidawan
Copy link
Member Author

Failing test:

FAILED lib/galaxy_test/api/test_item_tags.py::TestItemTagsApi::test_get_tags_histories_content - AssertionError: Request status code (403) was not expected value 200. Body was {'err_msg': 'HistoryDatasetAssociation is not owned by the current user', 'err_code': 403003}

seems unrelated to these changes. Otherwise, this should be good to review

@ElectronicBlueberry ElectronicBlueberry self-assigned this Apr 23, 2024
@ElectronicBlueberry
Copy link
Member

Just tested this, it worked fantastically. I like how the editor is linked to the job list. I did however notice a few things:

Here, I only had one input dataset, but it seems to be shown twice:
image


I think the job details list is a bit confusing, since it appears to be a dropdown, but isn't one.
I'd suggest the following as an alternative approach:

Always show the job details section. Either have it show the first job, with an option to view the next, or display a text explaining how to select what job to show. e.g. "To view Job details, select a job from the step summary" (If you go this route, also title the step summary, so users know what part of the UI you are referring to)

Instead of the dropdown button, have a button that says: View job details. If that's to much visual noise, you can hide the button (just visually) unless it's selected or the parent element is hovered.


It would be nice if the space of the details list could be changed, similarly to the side-bars. This is can however be done in a followup.


The contents of the tab seem to shifted to the right, and do not align with the rest of the page:

image


I'm not sure how I feel about multiple connection converging on the same node, but if it's difficult to separate, I'm fine with keeping it that way.

This could become tricky though with node output labels, if two tool outputs are not both of the same type.
If these can't be separated, I'm in favor of removing output node labels (the mouse-over ones) in this view.

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Apr 23, 2024

Thank you for taking a look @ElectronicBlueberry !

Here, I only had one input dataset, but it seems to be shown twice: ![image](https://private-user-images.githubusercontent.com/44241786/324792881-df8dea30-1c02-4178-b82c-121434b22051.png?

Yes, that part bothered me too since in the existing Steps tab, we already show the input dataset but then we also show it again when we expand the outputs section. And in the graph view we pre-expand these sections, so we show it twice. So I think I'll simply remove the redundant input dataset.

I think the job details list is a bit confusing, since it appears to be a dropdown, but isn't one. I'd suggest the following as an alternative approach:
Always show the job details section. Either have it show the first job, with an option to view the next, or display a text explaining how to select what job to show. e.g. "To view Job details, select a job from the step summary" (If you go this route, also title the step summary, so users know what part of the UI you are referring to)
Instead of the dropdown button, have a button that says: View job details. If that's to much visual noise, you can hide the button (just visually) unless it's selected or the parent element is hovered.

Yes, I am planning to modernize the whole Invocations List (into a grid similar to the WorkflowList) make some changes to the Invocations List (add a search bar maybe?...), and this also includes the full view for each individual invocation. Part of this is also the job details section and I can take what you've recommended here and do that in this PR.

It would be nice if the space of the details list could be changed, similarly to the side-bars. This is can however be done in a followup.

The contents of the tab seem to shifted to the right, and do not align with the rest of the page:

Ah yes, good spot. Fixing that.

I'm not sure how I feel about multiple connection converging on the same node, but if it's difficult to separate, I'm fine with keeping it that way.

Yes, it was the easiest way to keep the original structure without the labels since in this view we only care about showing the job state counts.

This could become tricky though with node output labels, if two tool outputs are not both of the same type. If these can't be separated, I'm in favor of removing output node labels (the mouse-over ones) in this view.

I think simply not showing the hover labels is fine

@ahmedhamidawan
Copy link
Member Author

It would be nice if the space of the details list could be changed, similarly to the side-bars. This is can however be done in a followup.

@ElectronicBlueberry Any ideas on what should be changed; should there be more space in between them, should the steps appear somewhere else, should we put the steps in the side panel (activity for individual invocations 🤷‍♂️)...?

@ElectronicBlueberry
Copy link
Member

I meant a drag handle to adjust the sizes dynamicaly

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Apr 23, 2024

I meant a drag handle to adjust the sizes dynamicaly

Ooh nice, that's a good idea

Update: Used the FlexPanel and it works really well!

@ahmedhamidawan ahmedhamidawan marked this pull request as draft April 24, 2024 04:25
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review April 25, 2024 19:35
@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Apr 25, 2024

I have made the several changes recommended by @ElectronicBlueberry and discussed in the UI/UX meeting, and have updated the screenshots/casts in the main PR description.

Would drag handles still be useful even though we can hide (toggle) the graph and see just the steps? Ended up using the FlexPanel; so now we have a drag handle

… steps if graph is collapsed, by removing the max-height
@bgruening
Copy link
Member

This is amazing @ahmedhamidawan! Awesome work.

What I find a bit confusing is that the "steps-list" is sorted from top to bottom, the latest step at the bottom of the list. But in our histories, it is the reverse, the latest "step" / job at the top. This is especially notable when you have history and step-list side by side.

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Apr 27, 2024

This is amazing @ahmedhamidawan! Awesome work.

What I find a bit confusing is that the "steps-list" is sorted from top to bottom, the latest step at the bottom of the list. But in our histories, it is the reverse, the latest "step" / job at the top. This is especially notable when you have history and step-list side by side.

Thank you @bgruening !!!
That's a very good point.
One thing though, what do we do to the input steps then, since they are the first steps?
Should they still remain at the top and the rest are in descending order?
Or should the inputs also go to the bottom?

@ElectronicBlueberry
Copy link
Member

This is amazing @ahmedhamidawan! Awesome work.

What I find a bit confusing is that the "steps-list" is sorted from top to bottom, the latest step at the bottom of the list. But in our histories, it is the reverse, the latest "step" / job at the top. This is especially notable when you have history and step-list side by side.

I'm unsure about this. Top to bottom is the order that corresponds to the workflow run form, and I would argue that the run form has more correlation to the step list, than the step list to the history. The steps only happen to line up with the history if the history was empty before and the steps only output one dataset. Even then, the names often don't match up. I think flipping the step order in this view would be more confusing than beneficial.

@mvdbeek
Copy link
Member

mvdbeek commented Apr 30, 2024

I think something went wrong at the end here, I now see this in the invocations list:

Screenshot 2024-04-30 at 16 53 07

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

This is so cool, I've been wanting something like this ever since I ran my first workflow on Galaxy!

@mvdbeek mvdbeek added the highlight Included in user-facing release notes at the top label Apr 30, 2024
@dannon dannon merged commit 516f541 into galaxyproject:dev Apr 30, 2024
55 checks passed
@ahmedhamidawan ahmedhamidawan deleted the graph_view_invocations branch April 30, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI-UX highlight Included in user-facing release notes at the top kind/feature
Projects
Development

Successfully merging this pull request may close these issues.

8 participants