[Frontend] Node detail view now can show workflow input/output artifacts #2305

eterna2 · 2019-10-04T12:14:15Z

This is part 1 of the PR with reference to #2172 - to show workflow input/output artifacts in the front-end.

cc: @Ark-kun

This PR:

Add a new component S3ArtifactLink which renders a <a href> link (supports only s3:// and minio://)
Updated DetailsTable:
- add a new optional valueComponent prop: will wrap the value if provided
- props is changed from string[][] to Array<[string?, any?]> so that the value need not be a string.
Updated StaticNodeDetails to include 2 additional fields: Input Artifacts and Output Artifacts.

The next PR will:

update the node backend with a new get artifact API, with additional query params (namely the endpoint, and secrets from the workflow status)
this endpoint should also support lookahead (i.e. if flag provided, will head the artifact size, and return either a link to the artifact or the actual content of the artifact if it is small)
this endpoint should support artifacts with and w/o gzip
It should probably also support partial download with size/length (and maybe offset/start), so that it's possible to visualize the beginning of large table. (@Ark-kun )

Video can be seen here:
https://drive.google.com/file/d/1GmNrJum20AdW3Bo0xDQJFIOvvMhCF7C_/view

This change is

k8s-ci-robot · 2019-10-04T12:14:29Z

Hi @eterna2. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Ark-kun · 2019-10-04T20:04:55Z

new get artifact API, with additional query params

It should probably also support partial download with size/length (and maybe offset/start), so that it's possible to visualize the beginning of large table.

Ark-kun · 2019-10-04T20:05:11Z

There are some frontend unit test errors:

https://travis-ci.com/kubeflow/pipelines/jobs/242030777

eterna2 · 2019-10-05T15:41:52Z

ok. updated the snapshot to match the current PR.

eterna2 · 2019-10-05T15:48:39Z

It should probably also support partial download with size/length (and maybe offset/start), so that it's possible to visualize the beginning of large table.
@Ark-kun

question. for ui-metadata it is not an issue, as the data type is specified.

However, for generic artifacts, there is no easy way to know what is the content? other than try/catch with a few possible choices? But probably doable, do a quick scan for number of commas, or common delimiters to differeniate between csv, json, or plain text.

Ark-kun · 2019-10-07T23:11:17Z

However, for generic artifacts, there is no easy way to know what is the content?

This PR will make the type information available to Frontend: #2323

But probably doable, do a quick scan for number of commas, or common delimiters to differeniate between csv, json, or plain text.

I think that's out of scope for this PR. Here we just want to restore the existing functionality. Initially we just want to show short strings. We can truncate the string to, say, 300 bytes and show something like "very long strin...", Size: 32167 bytes, URL: https://.....

Ark-kun · 2019-10-07T23:12:25Z

/ok-to-test

Ark-kun · 2019-10-09T20:32:54Z

It should probably also support partial download with size/length (and maybe offset/start), so that it's possible to visualize the beginning of large table. (@Ark-kun )

Note: I was talking about the Frontend server functionality. This partial download feature should not be exposed to the user.

Ark-kun · 2019-10-09T20:34:36Z

Are the artifact links in "clickable" - result in showing/downloading the artifact data?

eterna2 · 2019-10-10T00:00:36Z

Yup. It opens a new window and shows the data in plain text. I am just using the original get artifact API in the node backend as a HREF link in this PR (just a new window with a GET request).

However to trigger download instead of a new window, I need to add a Content-Disposition header to the API response.

Probably, can add a new query string flag download to set this.

Bobgy · 2019-10-10T01:41:00Z

I'll review this today.

Ark-kun · 2019-10-10T03:05:56Z

However to trigger download instead of a new window, I need to add a Content-Disposition header to the API response.

Probably, can add a new query string flag download to set this.

It's OK. People can always right-click the URL and use "Save as".

Ark-kun · 2019-10-10T03:10:23Z

I'll review this today.

Thank you.

Bobgy

@eterna2 thanks for the contribution!

LGTM to the overall implementation, added some nit comments.

frontend/src/components/S3ArtifactLink.tsx

Bobgy · 2019-10-10T10:26:57Z

frontend/src/lib/StaticGraphParser.ts

@@ -68,7 +68,7 @@ export function _populateInfoFromTemplate(info: SelectedNodeInfo, template?: Tem

  if (template.inputs) {
    info.inputs =
-      (template.inputs.parameters || []).map(p => [p.name, p.value || '']);
+      (template.inputs.parameters || []).map(p => [p.name, p.value || ''] as [string, string]);


nit: Can you give [string, string] a proper type/interface name and avoid casting here? Avoiding type casting helps with long term maintenance.

One way to avoid casting here is

....map((p): [string, string] => [p.name, p.value || '']);

example

What would be even better, we should probably upgrade typescript version to at least 3.5, so it can handle it by default. example

updated typescript to 3.6.

fixed issues introduced by the stricter typing by typescript >=3.5.

updated node to LTS 10 - because I notice a few peculiarity:

the grpc packages are not found in package-lock.json even thou it specified in package.json (I think is subsequently installed in the post-install step, but I am not sure if it is used in any other places) - so whenever I do a npm i they get included into the lock file. And this result in some issues for node 9.4, as some dependent packages seems to be missing (does not support node 9)? So I have to upgrade the docker images to node 10.

Some of packages are not following the semantic versioning faithfully - I have to freeze a couple of packages, as the latest ones breaks the app (when package-lock.json is updated when I upgraded typescript.

the grpc packages are not found in package-lock.json even thou it specified in package.json (I think is subsequently installed in the post-install step, but I am not sure if it is used in any other places) - so whenever I do a npm i they get included into the lock file. And this result in some issues for node 9.4, as some dependent packages seems to be missing (does not support node 9)? So I have to upgrade the docker images to node 10.

Thanks for the catch. I think the last committer forgot to update package-lock.json last time, let's just commit them so they are in sync. Besides upgrading to node 10 LTS is definitely a good choice.

Some of packages are not following the semantic versioning faithfully - I have to freeze a couple of packages, as the latest ones breaks the app (when package-lock.json is updated when I upgraded typescript.

Thanks for finding them, the solution sounds good to me.

Bobgy · 2019-10-10T10:27:23Z

frontend/src/components/S3ArtifactLink.tsx

+/**
+ * A component that renders an artifact link.
+ */
+const S3ArtifactLink: React.FC<S3Artifact> = (


May I ask for a really simple test for this component?

added 3 simple test case:

invalid artifact (empty bucket or key)

s3 artifact

minio artifact

Thanks! LGTM

frontend/src/components/S3ArtifactLink.tsx

Bobgy · 2019-10-10T10:36:53Z

Also, for convenience, can you record a video about how this looks like?
There are a lot of tools to do that, maybe you can try https://www.screencastify.com/

eterna2 · 2019-10-10T21:59:25Z

@Bobgy

List of changes:

Renamed S3ArtifactLink to MinioArtifactLink and added unit tests for 3 use cases - invalid, s3, and minio artifacts.
Upgraded typescript to 3.6 and fixed issues due to stricter type checking, added config to skip typecheck for material lib (as it breaks with new ts)
Upgraded docker image to use node 10 instead of node 9, because grpc is broken in node 9
freezed the version for a couple of packages because the latest in semantic version breaks the typechecks
add apache-2 LICENSE (his github used apache-2, but npm show ISC) for linear-layout-vector (seems to be a new dependency for react-virtualized)

video can be found at https://drive.google.com/file/d/1GmNrJum20AdW3Bo0xDQJFIOvvMhCF7C_/view

Ark-kun · 2019-10-10T22:45:31Z

/retest

Bobgy · 2019-10-11T08:47:45Z

Hi @eterna2, this is awesome! Thanks a lot for the quick change.

I hope I said this earlier because I didn't expect you to finish the upgrade so fast (which is awesome), but can you separate the following to a separate PR?

Upgraded typescript to 3.6 and fixed issues due to stricter type checking, added config to skip typecheck for material lib (as it breaks with new ts)

Upgraded docker image to use node 10 instead of node 9, because grpc is broken in node 9

freezed the version for a couple of packages because the latest in semantic version breaks the typechecks

add apache-2 LICENSE (his github used apache-2, but npm show ISC) for linear-layout-vector (seems to be a new dependency for react-virtualized)

Because this PR is getting too large and upgrading typescript brought too many side effects.

I would recommend finishing this PR without upgrading typescript (just use the pattern I introduced to avoid type casts).

....map((p): [string, string] => [p.name, p.value || '']);

Feel free to talk about your opinions because I can see you've already put a lot of efforts into this. I'm just giving suggestions that can give us more confidence of merging each of your PRs, and maybe sometimes I'm asking too much.

frontend/src/components/MinioArtifactLink.test.tsx

…o extract artifacts. Update view to render artifacts.

eterna2 · 2019-10-14T12:22:46Z

ok. fixed most of the minor stuff.

wonders whether do u want to setup an auto-formatter like prettier? Cuz I usually does that, and just auto-format everything as a pre-hook commit.

will fix the unit test and output later.

Bobgy · 2019-10-14T14:07:19Z

Thanks for suggestion, I am also planning on making a standard prettier config.

eterna2 · 2019-10-14T20:02:50Z

@Bobgy
Ok done with all the changes if I am not wrong.

Just a minor feedback, the unit test fails on node 9 (travis is running node 11, while dockerfile is using node 9) - might want to sync them up, cuz was puzzled by my failing unit tests until I went and look at travis and swapped to 11 (the errors are all datetime format diff with the snapshots).

But this can be specific to my machine thou. I am using wsl in windows.

Ark-kun · 2019-10-14T21:44:26Z

(travis is running node 11, while dockerfile is using node 9) - might want to sync them up

@Bobgy Which version should we standardize on: 9 ,10, 11 or 12 (In a different PR)? The NodeJS website lists 10 and 12 as LTS releases. Should we try to go with 12?

Bobgy · 2019-10-15T02:44:26Z

@Ark-kun Choosing latest LTS (12) is definitely the best option. I agree we should do it in a separate PR. I will create an issue for it.

Bobgy · 2019-10-15T02:56:22Z

@eterna2, thanks so much for this! This is awesome.

/lgtm
/approve

k8s-ci-robot · 2019-10-15T02:56:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

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:

~~frontend/OWNERS~~ [Bobgy]

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

Bobgy · 2019-10-15T03:34:46Z

/area front-end

k8s-ci-robot requested review from Bobgy and jingzhang36 October 4, 2019 12:14

k8s-ci-robot added the size/L label Oct 4, 2019

k8s-ci-robot added the needs-ok-to-test label Oct 4, 2019

eterna2 force-pushed the eterna2/frontend-artifact-view branch from dcd8b15 to bda3152 Compare October 5, 2019 17:52

Ark-kun assigned Bobgy Oct 7, 2019

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 7, 2019

Ark-kun added the priority/p0 label Oct 9, 2019

Bobgy reviewed Oct 10, 2019

View reviewed changes

k8s-ci-robot added size/XXL and removed size/L labels Oct 10, 2019

Bobgy reviewed Oct 11, 2019

View reviewed changes

frontend/src/components/MinioArtifactLink.test.tsx Show resolved Hide resolved

Added component to render link to artifact. Updated workflow parser t…

8b6c56a

…o extract artifacts. Update view to render artifacts.

eterna2 force-pushed the eterna2/frontend-artifact-view branch from e28dec7 to 8b6c56a Compare October 14, 2019 12:02

k8s-ci-robot added size/L and removed size/XXL labels Oct 14, 2019

revert package-lock.jsons

7a3cfc2

eterna2 force-pushed the eterna2/frontend-artifact-view branch 2 times, most recently from 2b7e57f to b6dd275 Compare October 14, 2019 12:18

minor styling fixes

c0f4202

eterna2 force-pushed the eterna2/frontend-artifact-view branch from b6dd275 to c0f4202 Compare October 14, 2019 12:20

Added WorkflowParser tests for getNodeInputOutputArtifacts

9df4ce0

k8s-ci-robot added size/XL and removed size/L labels Oct 14, 2019

eterna2 added 2 commits October 15, 2019 02:43

fix typo

0672a5c

Add test for DetailsTable.valueComponent

93afe33

eterna2 added 2 commits October 15, 2019 04:10

Fix types for values passed into DetailsTable

117e485

fix minor style errors

d834480

k8s-ci-robot added the lgtm label Oct 15, 2019

k8s-ci-robot added the approved label Oct 15, 2019

Bobgy mentioned this pull request Oct 15, 2019

[Frontend] upgrade node to 12 LTS #2386

Closed

2 tasks

k8s-ci-robot added the area/frontend label Oct 15, 2019

k8s-ci-robot merged commit e8477ff into kubeflow:master Oct 15, 2019

Bobgy mentioned this pull request Oct 18, 2019

[Frontend] Upgrade typescript to 3.6 #2428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend] Node detail view now can show workflow input/output artifacts #2305

[Frontend] Node detail view now can show workflow input/output artifacts #2305

eterna2 commented Oct 4, 2019 •

edited

Loading

k8s-ci-robot commented Oct 4, 2019

Ark-kun commented Oct 4, 2019

Ark-kun commented Oct 4, 2019

eterna2 commented Oct 5, 2019

eterna2 commented Oct 5, 2019 •

edited

Loading

Ark-kun commented Oct 7, 2019 •

edited

Loading

Ark-kun commented Oct 7, 2019

Ark-kun commented Oct 9, 2019

Ark-kun commented Oct 9, 2019

eterna2 commented Oct 10, 2019

Bobgy commented Oct 10, 2019

Ark-kun commented Oct 10, 2019

Ark-kun commented Oct 10, 2019

Bobgy left a comment

Bobgy Oct 10, 2019

Bobgy Oct 10, 2019

eterna2 Oct 10, 2019

eterna2 Oct 10, 2019

Bobgy Oct 11, 2019

Bobgy Oct 10, 2019

eterna2 Oct 10, 2019

Bobgy Oct 11, 2019

Bobgy commented Oct 10, 2019

eterna2 commented Oct 10, 2019 •

edited

Loading

Ark-kun commented Oct 10, 2019

Bobgy commented Oct 11, 2019

eterna2 commented Oct 14, 2019

Bobgy commented Oct 14, 2019

eterna2 commented Oct 14, 2019

Ark-kun commented Oct 14, 2019 •

edited

Loading

Bobgy commented Oct 15, 2019

Bobgy commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

Bobgy commented Oct 15, 2019

[Frontend] Node detail view now can show workflow input/output artifacts #2305

[Frontend] Node detail view now can show workflow input/output artifacts #2305

Conversation

eterna2 commented Oct 4, 2019 • edited Loading

k8s-ci-robot commented Oct 4, 2019

Ark-kun commented Oct 4, 2019

Ark-kun commented Oct 4, 2019

eterna2 commented Oct 5, 2019

eterna2 commented Oct 5, 2019 • edited Loading

Ark-kun commented Oct 7, 2019 • edited Loading

Ark-kun commented Oct 7, 2019

Ark-kun commented Oct 9, 2019

Ark-kun commented Oct 9, 2019

eterna2 commented Oct 10, 2019

Bobgy commented Oct 10, 2019

Ark-kun commented Oct 10, 2019

Ark-kun commented Oct 10, 2019

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Oct 10, 2019

eterna2 commented Oct 10, 2019 • edited Loading

Ark-kun commented Oct 10, 2019

Bobgy commented Oct 11, 2019

eterna2 commented Oct 14, 2019

Bobgy commented Oct 14, 2019

eterna2 commented Oct 14, 2019

Ark-kun commented Oct 14, 2019 • edited Loading

Bobgy commented Oct 15, 2019

Bobgy commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

Bobgy commented Oct 15, 2019

eterna2 commented Oct 4, 2019 •

edited

Loading

eterna2 commented Oct 5, 2019 •

edited

Loading

Ark-kun commented Oct 7, 2019 •

edited

Loading

eterna2 commented Oct 10, 2019 •

edited

Loading

Ark-kun commented Oct 14, 2019 •

edited

Loading