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

feat(frontend): Add a new page to monitor existing Jobs (recurring runs) #5131

Merged
merged 15 commits into from Mar 4, 2021

Conversation

StefanoFioravanzo
Copy link
Member

This PR introduces a new page to visualize the existing Jobs (recurring runs). This page allows to

  • Navigate to the (already existing) Job Details page, by clicking on the Job name. From there it's possible to enable or disable the Job, and have an overview of the Job's settings
  • Navigate to the Experiment page to view the Job's pipeline Runs, by clicking on the Experiment name
  • See at a glance the status of the Job and its time schedule

Before this PR, the only way to manage and monitor Jobs was to navigate to an Experiment page and from there access to the Jobs for that Experiment only. This is clearly cumbersome when there are multiple Jobs scattered across multiple Experiments.

Screenshot 2021-02-12 at 11 30 46

More specifically, the PR introduces two new pages:

  • JobList: inspired by RunList, it lists all Jobs (Recurring Runs) with some useful info about each Job (status, trigger, related Experiment, creation date)
  • AllJobsList: inspired by AllRunsList, it renders JobList + adds the page title and contextual toolbar

There are some commonalities especially between JobList and RunList. @Bobgy let me know what you think about that, we could think about moving some of these functions to a common place. They are almost identical but differ from the type or some fields (e.g. _getAndSetExperimentNames differs just on the input type and then displayJob.job vs displayRun.run). Maybe we can generalize this in a nice way. I just "copied" these functions for now, for the sake of simplicity.

Checklist:

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: StefanoFioravanzo
To complete the pull request process, please assign bobgy after the PR has been reviewed.
You can assign the PR to them by writing /assign @bobgy in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@StefanoFioravanzo
Copy link
Member Author

Also @Bobgy I had some trouble making this test work:
https://github.com/kubeflow/pipelines/pull/5131/files#diff-3fa1b586a7cee2480a4e123899257b8924ad3dddc29e560f6bf6c9a5fd520a64R76-R83

I couldn't figure out how to set the spy on the child component

@StefanoFioravanzo
Copy link
Member Author

@Bobgy I went through #5136 , it's great and I totally agree with what you propose there. Still, since I have already done the work here, it would be a shame to re-implementing (almost) from scratch. I share the felling that we need to start using our best-practices sooner rather than later, but if you agree can we make an exception here?

Copy link
Collaborator

@zijianjoy zijianjoy left a comment

Choose a reason for hiding this comment

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

Thank you Stefan for adding this jobs page!

frontend/src/pages/JobList.tsx Outdated Show resolved Hide resolved
frontend/src/pages/JobList.tsx Outdated Show resolved Hide resolved
frontend/src/pages/JobList.tsx Outdated Show resolved Hide resolved
frontend/src/pages/JobList.tsx Outdated Show resolved Hide resolved
@Bobgy
Copy link
Contributor

Bobgy commented Feb 16, 2021

@Bobgy I went through #5136 , it's great and I totally agree with what you propose there. Still, since I have already done the work here, it would be a shame to re-implementing (almost) from scratch. I share the felling that we need to start using our best-practices sooner rather than later, but if you agree can we make an exception here?

Totally understood, because you already wrote the code, it's not worth it rewriting it.

We can start later

I am on vacation, will take a look when I'm back.

/assign @zijianjoy
To review first

@Bobgy Bobgy added this to In progress in Kubeflow 1.3 release via automation Feb 20, 2021
@Bobgy Bobgy moved this from In progress to P1 in Kubeflow 1.3 release Feb 20, 2021
frontend/src/pages/AllJobsList.test.tsx Outdated Show resolved Hide resolved
frontend/src/pages/JobList.tsx Outdated Show resolved Hide resolved
frontend/src/components/Router.tsx Outdated Show resolved Hide resolved
frontend/src/components/SideNav.test.tsx Outdated Show resolved Hide resolved
@Bobgy
Copy link
Contributor

Bobgy commented Feb 20, 2021

Thank you again, this is a super useful for users!
Really appreciate your efforts!

Introduce two new pages:

- JobList: inspired by RunList, it lists all Jobs (Recurring Runs)
  with some useful info about each Job (status, trigger, related
  Experiment, creation date)
- AllJobsList: inspired by AllRunsList, it renders JobList + adds
  the page title and contextual toolbar

Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Copy link
Contributor

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

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

I don't have major concerns with the code.

Next step is to resolve the naming issue

frontend/src/pages/AllJobsList.test.tsx Outdated Show resolved Hide resolved
@StefanoFioravanzo
Copy link
Member Author

I don't have major concerns with the code.

Next step is to resolve the naming issue

@Bobgy I will push a commit resolving the naming issue shortly. Regarding the refresh test, could you explain to me why the who page is not rendered? Here da09a95#diff-3fa1b586a7cee2480a4e123899257b8924ad3dddc29e560f6bf6c9a5fd520a64R34 I am also calling getInitialToolbarState, so I would expect the toolbar to be rendered. I am definitely missing something as to how render works

Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
@Bobgy
Copy link
Contributor

Bobgy commented Mar 1, 2021

RoutedPage is where the page level common elements are rendered in KFP UI. However, in tests, we built a util that generate similar page callbacks and pass them to the tested component without actually rendering the page common elements.

@StefanoFioravanzo
Copy link
Member Author

Is this ready for merging?

Copy link
Collaborator

@zijianjoy zijianjoy left a comment

Choose a reason for hiding this comment

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

Leaves two comments, the rest look good to me. Thank you so much!

Comment on lines +132 to +136
// it('refreshes the recurring run list when refresh button is clicked', async () => {
// const tree = render(<AllRecurringRunsList {...generateProps()} />);
// await TestUtils.flushPromises()
// fireEvent.click(tree.getByText('Refresh'));
// });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this commented block if test is not enabled, or add a TODO to explain why and plan for enabling it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zijianjoy You are right. I added a TODO above the comment so we don't lose context and we can come back to it later on

@@ -31,6 +31,7 @@ import AllExperimentsAndArchive, {
AllExperimentsAndArchiveTab,
} from '../pages/AllExperimentsAndArchive';
import AllRunsAndArchive, { AllRunsAndArchiveTab } from '../pages/AllRunsAndArchive';
import AllJobsList from '../pages/AllRecurringRunsList';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be AllRecurringRunsList instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@zijianjoy Good catch! Fixed it

Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
Signed-off-by: Stefano Fioravanzo <stefano@arrikto.com>
@Bobgy
Copy link
Contributor

Bobgy commented Mar 4, 2021

/lgtm
/approve

Thank you @StefanoFioravanzo!
Love this new page

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, StefanoFioravanzo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit 4ff3ffb into kubeflow:master Mar 4, 2021
Kubeflow 1.3 release automation moved this from P1 to Done Mar 4, 2021
@zijianjoy
Copy link
Collaborator

Thank you @StefanoFioravanzo for the change!

@elikatsis elikatsis deleted the feature-stefano-jobs-page branch March 12, 2021 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants