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

Place invocation state in list #17708

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

jmchilton
Copy link
Member

The actual invocation state we decided wasn't very useful at the team meeting. Foregrounding the progress bars is a potential improvement. I've done some query optimization for this.

Screen.Recording.2024-03-13.at.5.20.19.PM.mov

How to test the changes?

(Select all options that apply)

  • Instructions for manual testing are as follows:
    1. Load up some invocations and look at the grid.

License

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

@ahmedhamidawan
Copy link
Member

ahmedhamidawan commented Mar 13, 2024

I have also added modifications to the Summary tab here: #17413, which will work nicely with these changes!

@jmchilton jmchilton force-pushed the place_state_in_invocation_list branch from 00de142 to ec403cc Compare March 25, 2024 17:55
@jmchilton jmchilton force-pushed the place_state_in_invocation_list branch from b47999a to 31d9b48 Compare April 3, 2024 17:12
@jmchilton jmchilton marked this pull request as ready for review April 3, 2024 19:40
@github-actions github-actions bot added this to the 24.1 milestone Apr 3, 2024
@jmchilton
Copy link
Member Author

Errors seem related - might close this out and get smaller pieces merged first to narrow things down.

@jmchilton jmchilton marked this pull request as draft April 16, 2024 12:52
@jmchilton jmchilton force-pushed the place_state_in_invocation_list branch from 31d9b48 to 1f0731e Compare April 17, 2024 15:51
@jmchilton jmchilton mentioned this pull request Apr 17, 2024
2 tasks
@mvdbeek mvdbeek removed this from the 24.1 milestone May 14, 2024
@jmchilton
Copy link
Member Author

@ahmedhamidawan I stopped working on this because I thought your work superseded it but @mvdbeek mentioned maybe this was still useful. If that is the case, let me know and I can try to rebase it against your latest work or alternatively feel free to pull the bits and pieces of out of this that are useful if any of them are.

@ahmedhamidawan
Copy link
Member

ahmedhamidawan commented Oct 24, 2024

This is waiting on #18615

@ahmedhamidawan ahmedhamidawan force-pushed the place_state_in_invocation_list branch from 1f0731e to b06cc8e Compare November 7, 2024 00:32
@ahmedhamidawan ahmedhamidawan force-pushed the place_state_in_invocation_list branch from b06cc8e to bb96e84 Compare November 7, 2024 00:43
@@ -169,7 +173,7 @@ export function useInvocationGraph(
invocationStepSummary = stepsJobsSummary.find((stepJobSummary: StepJobSummary) => {
if (stepJobSummary.model === "ImplicitCollectionJobs") {
return stepJobSummary.id === invocationStep.implicit_collection_jobs_id;
} else {
} else if (stepJobSummary.model === "Job") {
return stepJobSummary.id === invocationStep.job_id;
Copy link
Member

Choose a reason for hiding this comment

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

@jmchilton As you can see in this 78f653c commit, I needed to use LegacyWorkflowInvocationElementView because right here, we need the job_id to associate the stepJobSummary with it.
Of course, overall, I need to rebase and readjust these changes, but is this the right approach as far as the view is concerned?

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.

3 participants