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

Analysis schema for historic job data #530

Merged
merged 12 commits into from
Mar 8, 2024

Conversation

ChrisPaulBennett
Copy link
Contributor

@ChrisPaulBennett ChrisPaulBennett commented Nov 29, 2023

Linked to cylc/cylc-ui#1510 and cylc/cylc-ui#1466

This is a branch for querying historic job data. You can give it a list of task names to limit which jobs are returned, otherwise it will default to all jobs.

image

This is a quick work in progress branch that I have used for development of UI views to visualise job timing data and wants tidying up before it is ready to go on master."

I have taken ownership of this issue (Previously #511)

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).
  • Tests are included (or explain why tests are not needed).
  • No changelog entry needed (UI PRs are the user-facing ones)
  • Nod docs PR needed
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@ChrisPaulBennett ChrisPaulBennett marked this pull request as ready for review December 1, 2023 11:11
@ChrisPaulBennett
Copy link
Contributor Author

I have refactored some code and added some extra tests.
Not sure why the MacOS (Why does this exist?) tests are failing. It's in a set of tests that I haven't touched.
Any input on this would be appreciated.

@MetRonnie MetRonnie self-requested a review December 1, 2023 15:08
@MetRonnie
Copy link
Member

MacOS tests failures unrelated - actions/runner-images#8649

@MetRonnie MetRonnie added this to the 1.4.4 milestone Dec 1, 2023
@MetRonnie
Copy link
Member

@oliver-sanders I think this needs to be on 1.4.x right?

@MetRonnie MetRonnie changed the title Analysis schema Analysis schema for historic job data Dec 5, 2023
@oliver-sanders oliver-sanders modified the milestones: 1.4.4, 1.5.0 Dec 5, 2023
@oliver-sanders
Copy link
Member

Changed the milestone to 1.5.0

@MetRonnie
Copy link
Member

MetRonnie commented Dec 5, 2023

This is not going to be available for Jupyter Server 2 users? Never mind, I got confused

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.

LGTM

cylc/uiserver/schema.py Outdated Show resolved Hide resolved
@MetRonnie
Copy link
Member

MetRonnie commented Dec 5, 2023

I've just force-pushed to simplify the commit history. No changes made.

image

If any more changes are needed on this branch, please reset your local branch to this latest commit and then add commits on top of it (rather than merging this into your local branch, which will undo the commit history simplification)

@MetRonnie MetRonnie mentioned this pull request Dec 5, 2023
8 tasks
@oliver-sanders oliver-sanders reopened this Dec 5, 2023
@MetRonnie
Copy link
Member

I've just force-pushed to simplify the commit history. No changes made.

image

If any more changes are needed on this branch, please reset your local branch to this latest commit and then add commits on top of it (rather than merging this into your local branch, which will undo the commit history simplification)

I've done this again, please follow these instructions if you want to add any more commits

@ChrisPaulBennett
Copy link
Contributor Author

All points have been resolved. The lack of test coverage is due to the function list_elements being troublesome to test.
This function access's a SQL DB that exists in a workflow directory. I'm currently looking for a way to point it at a test data directory.
If you currently have a method, that would be appreciated

@MetRonnie
Copy link
Member

I'm not so concerned by the lack of coverage on list_elements(). But maybe get_elements() could do with testing. You could use Pytest's monkeypatch fixture to replace list_elements with a mock in order to test the expected output of get_elements without running any DB queries

@MetRonnie
Copy link
Member

Got some suggestions at ChrisPaulBennett#5

Test `get_elements()` more thoroughly
@ChrisPaulBennett
Copy link
Contributor Author

You said that you were not concerned about list_elements().
Are you happy with this pull request now?

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.

LGTM, suggest keeping this open whilst the UI PR is worked on in case changes are required.

@oliver-sanders

This comment was marked as off-topic.

@oliver-sanders
Copy link
Member

Upstream PR is in, getting the tests passing before merging this one.

@oliver-sanders oliver-sanders merged commit d075928 into cylc:master Mar 8, 2024
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants