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

Add analysis view table containing workflow timing statistics #1254

Merged
merged 26 commits into from
Apr 24, 2023

Conversation

JAllen42
Copy link
Contributor

@JAllen42 JAllen42 commented Feb 28, 2023

Partially addresses #1170
Linked with cylc/cylc-uiserver#434
Based on the proof of concept #1171

This adds a table view containing statistics of the timings of tasks in a workflow. Currently this includes mean, standard deviation, minimum, maximum, median and quartiles, which can be calculated using the queue times for tasks, run times or the total time. These can be filtered based on the name of the task and which platform the tasks was run on.

image

The proof of concept query which loads job information from the workflow database has been changed to load task information instead, and the subscription has been deleted and replaced with a button to rerun the query.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good 👍

src/services/workflow.service.js Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

This branch is now a little behind master and will require rebasing at some point:

$ git fetch upstream
$ git checkout analysis_view
$ git branch analysis_view.safe  # make a safe copy before starting
$ git rebase upstream/master
$ # fix any conflicts flagged during rebase
$ git add -u && git rebase --continue  # if there were any conflicts
$ git push -f origin HEAD

@JAllen42 JAllen42 marked this pull request as ready for review March 23, 2023 16:58
@JAllen42
Copy link
Contributor Author

This has been rebased and restructured slightly to make it easier to add the tests.

There might need to be a couple of minor changes if the structure of the data is tweaked in the sibling ui-server branch, but other than that I think this is ready for review

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Looks good! I still need to test with a real workflow

@JAllen42 You need to add an entry to .mailmap, something like

Jamie Allen       <your.metoffice@email.address>      JAllen42     <your.git.commit@email.address>

src/components/cylc/analysis/filter.js Outdated Show resolved Hide resolved
name: 'analysis',
meta: {
layout: 'default',
toolbar: true
Copy link
Member

Choose a reason for hiding this comment

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

The convention for these standalone views is to hide the sidebar, unless there is a reason not to in this case?

Suggested change
toolbar: true
toolbar: true,
showSidebar: false

src/services/workflow.service.js Outdated Show resolved Hide resolved
src/utils/tasks.js Outdated Show resolved Hide resolved
src/views/Analysis.vue Outdated Show resolved Hide resolved
src/views/Analysis.vue Outdated Show resolved Hide resolved
src/views/Analysis.vue Outdated Show resolved Hide resolved
src/views/Analysis.vue Outdated Show resolved Hide resolved
.mailmap Outdated Show resolved Hide resolved
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

🚀

@MetRonnie MetRonnie changed the base branch from master to 1.6.x April 6, 2023 16:16
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, works smoothly, nice!

Pagination, filtering, timing and platform selection all work well.

I tried being an impatient user hammering the refresh button and managed to find one small issue where repeat presses can cause duplicate tasks.

Press once:

Screenshot from 2023-04-14 14-42-57

Press three times:

Screenshot from 2023-04-14 14-43-20

There's a neat trick to handle users hammering buttons called debounce which you can find in the lodash library.

There's an example usage in this PR in src/views/Log.vue. You wrap your function with debounce, then duplicate calls within the specified timeframe will effectively be ignored:

methods: {
  historicalQuery: debounce(
    async function () {
      this.tasks = []
      ...
    },
    500  // only call this once every 500ms
  }
}

This article provides an explanation: https://css-tricks.com/debouncing-throttling-explained-examples/#aa-debounce

This is relatively minor so not a blocker, but if possible it would be good to get this debounced befure it shows up in the wild.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Functional review: an excellent addition to Cylc - thanks @JAllen42 and @ChrisPaulBennett

(I'll hold off merging till the debounce question is addressed).

@oliver-sanders
Copy link
Member

oliver-sanders commented Apr 21, 2023

I'm afraid you've picked up a couple more conflicts, rebase instructions or alternatively use the "resolve conflicts" button below (note if use use the button the commit won't appear automatically in your local branch)

@oliver-sanders
Copy link
Member

Cheers!

@oliver-sanders oliver-sanders merged commit 3ce61a8 into cylc:1.6.x Apr 24, 2023
MetRonnie added a commit to MetRonnie/cylc-ui that referenced this pull request May 31, 2023
MetRonnie added a commit that referenced this pull request May 31, 2023
@MetRonnie MetRonnie mentioned this pull request May 31, 2023
6 tasks
@MetRonnie MetRonnie added the data workflows team Work pertaining to the analysis/gantt/etc views label Apr 9, 2024
@MetRonnie MetRonnie mentioned this pull request Apr 9, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data workflows team Work pertaining to the analysis/gantt/etc views
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants