UI Support for PD #5326

YintongMa · 2021-07-29T22:45:20Z

AoU UI Preview

https://projects.invisionapp.com/d/main#/console/21502769/454452703/preview

Terra UI Preview

https://app.terra.bio/#workspaces/terra-dev-myt/terra-dev/notebooks/launch/terra-dev-notebook.ipynb

Demo

Gce with PD panel

Dataproc panel

yonghaoy

First path on API changes, haven't review UI change yet. Few general nit:

There are many copy-paste errors from runtime -> persistent disk
I saw some test breakage caused by this change.
No unit test for API changes. Adding tests is generally helpful.

api/src/main/java/org/pmiops/workbench/api/DiskController.java

api/src/main/resources/workbench-api.yaml

ui/src/app/pages/analysis/runtime-panel.tsx

api/src/main/java/org/pmiops/workbench/api/DiskController.java

api/src/main/java/org/pmiops/workbench/notebooks/LeonardoNotebooksClient.java

ui/src/app/utils/stores.tsx

ui/src/app/pages/analysis/runtime-panel.tsx

api/config/config_test.json

api/src/main/java/org/pmiops/workbench/api/DiskController.java

yonghaoy · 2021-08-03T21:21:56Z

api/src/main/java/org/pmiops/workbench/api/DiskController.java

+      LeonardoGetPersistentDiskResponse response;
+      List<LeonardoListPersistentDiskResponse> responseList =
+          leonardoNotebooksClient.listPersistentDiskByProject(googleProject, false);
+      responseList.sort(


How you guarantee the last created disk is the one we want? 10 users are using the same workspace, and create PDs at the same time. What would happen then?

calbach

Sorry for the delayed reviews, will look at this today but possibly not until tonight.

ui/src/app/utils/runtime-utils.tsx

yonghaoy

Thanks! looks simpler now!

ui/src/app/pages/analysis/runtime-panel.tsx

calbach

The API looks good - a few cleanups suggested. Can we split the UI changes out into a separate PR? It seems like it should be totally separable to me as there are no breaking API changes here.

api/src/main/java/org/pmiops/workbench/api/DiskController.java

api/src/main/java/org/pmiops/workbench/api/RuntimeController.java

api/src/main/resources/workbench-api.yaml

api/src/main/java/org/pmiops/workbench/api/DiskController.java

calbach

Note: you will need to merge or rebase with the Angular->React - likely it just affects the diskStore initialization.

Do you have the runtime panel spec changes?

ui/src/app/pages/analysis/runtime-panel.tsx

ui/src/app/utils/runtime-utils.tsx

calbach · 2021-08-19T01:25:51Z

ui/src/app/utils/runtime-utils.tsx

    return {
      computeType: ComputeType.Standard,
      machine: findMachineByName(runtime.gceConfig.machineType),
      diskSize: runtime.gceConfig.diskSize,


hm, we still have diskSize after this change. WIll that always be set?

Note: you will need to merge or rebase with the Angular->React - likely it just affects the diskStore initialization.

Sorry, do you mean I need to merge/rebase with the latest master since there are some codebase changes of Angular->React ?

Do you have the runtime panel spec changes?

No, there are some code format diffs with the master after I run yarn lint --fix ... I have to revert them manually.

ui/src/app/pages/analysis/runtime-panel.tsx

calbach

Thanks for adding screenshots. I see the PD cost estimator which is cool, but I don't see the commits in this PR. Do you have commits to push still?

ui/src/app/pages/analysis/runtime-panel.tsx

calbach

OK - if this is working, I think I'm about good to approve pending remaining comments. RE moving logic to backend: it isn't a bad idea but I don't see a clear path so this may be a bit risky. I also definitely want to play around with this a bit - and definitely want to get Shimon/Lou feedback on the new UX interactions, e.g. the separate PD disk estimator.

ui/src/app/utils/runtime-utils.tsx

calbach · 2021-08-20T02:10:51Z

Sorry, I lost my other comment: it was to revert submodules, and apologies for the additional merge conflicts here.

yonghaoy · 2021-08-20T02:52:59Z

OK - if this is working, I think I'm about good to approve pending remaining comments. RE moving logic to backend: it isn't a bad idea but I don't see a clear path so this may be a bit risky. I also definitely want to play around with this a bit - and definitely want to get Shimon/Lou feedback on the new UX interactions, e.g. the separate PD disk estimator.

If rewriting most of this code to move those logic to backend is too hard/frustrate, you can leave this in frontend for now. I think Leo is working on writing this API as well, it might be throw away effort if we make API change now. One thing that can be a further improvement is asking Leo to change their API interface for one API (I remember there is a PD related API but they use UpdateGceConfig?) .
The UI change is good for now. I believe after the existing runtime get auto deleted, the logic would be simpler.

This is great! Thanks!

yonghaoy

Since this is proteched by feature branch, I think we can go with this, and polish if needed in following PR

YintongMa · 2021-08-21T03:10:29Z

The test fails after i merge latest master : ERROR in src/app/pages/app/component.ts(35,29): error TS2307: Cannot find module 'app/services/sign-in.service'.

calbach

I'm out next week, so don't wait on my approval - this is fine to merge once the rebase issues are addressed

calbach · 2021-08-23T03:16:51Z

ui/src/app/pages/app/component.ts

@@ -0,0 +1,310 @@
+import {Component, NgZone, OnDestroy, OnInit} from '@angular/core';


This file was removed, you'll need to move your logic to somewhere around here: https://github.com/all-of-us/workbench/blob/master/ui/src/app/routing/app-routing.tsx#L272

merge master

Yintong Ma added 7 commits July 28, 2021 16:24

UUID support

ca68141

remove UUID for runtime

ddf1565

rm useless pkg

51d65d0

code format

dd6b486

add pd controller

b673861

add crud UI support

9ddc0d5

fix corner cases

54021aa

YintongMa closed this Jul 30, 2021

YintongMa reopened this Jul 30, 2021

Yintong Ma added 3 commits July 31, 2021 06:20

code refactor

3a9498f

format code

6852ea2

add disk mapper

84aab8b

YintongMa requested review from calbach and yonghaoy August 2, 2021 16:10

yonghaoy reviewed Aug 2, 2021

View reviewed changes

ui/src/app/utils/stores.tsx Show resolved Hide resolved

ui/src/app/pages/analysis/runtime-panel.tsx Outdated Show resolved Hide resolved

ui/src/app/pages/analysis/runtime-panel.tsx Show resolved Hide resolved

Yintong Ma added 3 commits August 3, 2021 19:17

add feature flag enablePersistentDisk

5200f78

elaborate Disk API

33082c9

rmv useless code

5f5f99d

YintongMa requested a review from yonghaoy August 3, 2021 21:05

yonghaoy reviewed Aug 3, 2021

View reviewed changes

Yintong Ma added 2 commits August 3, 2021 22:16

merge master

6de9588

enable test env

944f130

calbach reviewed Aug 4, 2021

View reviewed changes

yonghaoy reviewed Aug 4, 2021

View reviewed changes

ui/src/app/utils/runtime-utils.tsx Outdated Show resolved Hide resolved

ui/src/app/utils/runtime-utils.tsx Outdated Show resolved Hide resolved

ui/src/app/utils/runtime-utils.tsx Outdated Show resolved Hide resolved

ui/src/app/utils/runtime-utils.tsx Outdated Show resolved Hide resolved

getDisk by user && simplify UI changes

322d38f

yonghaoy reviewed Aug 5, 2021

View reviewed changes

ui/src/app/pages/analysis/runtime-panel.tsx Show resolved Hide resolved

ui/src/app/pages/analysis/runtime-panel.tsx Outdated Show resolved Hide resolved

ui/src/app/pages/analysis/runtime-panel.tsx Outdated Show resolved Hide resolved

resolve conflicts with dataproc panel

d3c4010

calbach reviewed Aug 5, 2021

View reviewed changes

api/src/main/java/org/pmiops/workbench/api/DiskController.java Outdated Show resolved Hide resolved

code refactor & resize pd selector

7aaa51c

calbach reviewed Aug 19, 2021

View reviewed changes

fix bugs of panel switching between dataproc and running gce without PD

b6f47b5

YintongMa commented Aug 19, 2021

View reviewed changes

ui/src/app/pages/analysis/runtime-panel.tsx Show resolved Hide resolved

Yintong Ma added 3 commits August 19, 2021 02:53

fix enablePD usage

e5cf619

fix bug

50c3370

fix bug & add comments

c3d7fff

YintongMa requested review from calbach and yonghaoy August 19, 2021 18:44

calbach reviewed Aug 20, 2021

View reviewed changes

ui/src/app/utils/runtime-utils.tsx Outdated Show resolved Hide resolved

yonghaoy approved these changes Aug 20, 2021

View reviewed changes

Yintong Ma added 6 commits August 20, 2021 22:23

use context wrapper to simplify complex state predicate

faefaa9

fix unit tests bugs

7aa8c92

rm standardPdValidator

675a944

merge master

47afd21

fix runtime-utils-spec bug

ba4038d

revert serviceaccounts.rb

e8d27cb

YintongMa requested review from calbach and yonghaoy August 21, 2021 03:07

calbach reviewed Aug 23, 2021

View reviewed changes

Yintong Ma added 4 commits August 24, 2021 19:48

fix angular issue

69d7819

merge master

7519b68

Merge branch 'master' into yintong/UIForPD

01ec952

merge master

fix lint issue

ce955c6

YintongMa merged commit 6818a55 into master Aug 25, 2021

YintongMa deleted the yintong/UIForPD branch August 25, 2021 21:21

		@@ -0,0 +1,310 @@
		import {Component, NgZone, OnDestroy, OnInit} from '@angular/core';

UI Support for PD #5326

UI Support for PD #5326

Uh oh!

Conversation

YintongMa commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AoU UI Preview

Terra UI Preview

Demo

Gce with PD panel

Dataproc panel

Uh oh!

yonghaoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yonghaoy Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yonghaoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calbach Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

YintongMa Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

YintongMa commented Jul 29, 2021 •

edited

Loading