fix(frontend): fix parsing large workflow graph. Fixes #4179 #4180

radcheb · 2020-07-08T19:17:56Z

Description of your changes:
Fixes #4179

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.
Do you want this pull request (PR) cherry-picked into the current release branch?

If yes, use one of the following options:
(Recommended.) Ask the PR approver to add the cherrypick-approved label to this PR. The release manager adds this PR to the release branch in a batch update.

googlebot · 2020-07-08T19:18:05Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2020-07-08T19:18:07Z

Hi @radcheb. 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.

radcheb · 2020-07-08T20:54:21Z

@googlebot I signed it!

googlebot · 2020-07-08T20:54:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

kubeflow-bot · 2020-07-08T20:54:26Z

This change is

Ark-kun · 2020-07-08T21:26:56Z

/ok-to-test

Ark-kun · 2020-07-08T22:14:31Z

/retest

Bobgy

Thank you @radcheb for the contribution!

Just one comment. The implementation looks great!
/lgtm

frontend/src/pages/RunDetails.tsx

radcheb · 2020-08-09T17:30:22Z

/retest

Bobgy · 2020-08-09T23:15:37Z

The failed tests had an error building frontend image.

Step #2 - "frontend": Failed to compile.

14886

Step #2 - "frontend":

14887

Step #2 - "frontend": /src/frontend/src/lib/Utils.tsx

14888

Step #2 - "frontend": TypeScript error in /src/frontend/src/lib/Utils.tsx(379,5):

14889

Step #2 - "frontend": Cannot find name 'reject'. TS2304

14890

Step #2 - "frontend":

14891

Step #2 - "frontend": 377 | } catch (err) {

14892

Step #2 - "frontend": 378 | logger.error('Error decoding compressedNodes!', err);

14893

Step #2 - "frontend": > 379 | reject(err);

14894

Step #2 - "frontend": | ^

14895

Step #2 - "frontend": 380 | }

14896

Step #2 - "frontend": 381 | }

14897

Step #2 - "frontend": 382 |

radcheb · 2020-08-10T07:45:35Z

The failed tests had an error building frontend image.

Step #2 - "frontend": Failed to compile.

14886

Step #2 - "frontend":

14887

Step #2 - "frontend": /src/frontend/src/lib/Utils.tsx

14888

Step #2 - "frontend": TypeScript error in /src/frontend/src/lib/Utils.tsx(379,5):

14889

Step #2 - "frontend": Cannot find name 'reject'. TS2304

14890

Step #2 - "frontend":

14891

Step #2 - "frontend": 377 | } catch (err) {

14892

Step #2 - "frontend": 378 | logger.error('Error decoding compressedNodes!', err);

14893

Step #2 - "frontend": > 379 | reject(err);

14894

Step #2 - "frontend": | ^

14895

Step #2 - "frontend": 380 | }

14896

Step #2 - "frontend": 381 | }

14897

Step #2 - "frontend": 382 |

Ok, thanks @Bobgy for your response, I look at it.

radcheb · 2020-08-13T20:02:18Z

/retest

radcheb · 2020-08-13T20:05:28Z

/test kubeflow-pipeline-frontend-test

Bobgy · 2020-08-14T08:42:05Z

@radcheb would you mind reformatting the code?
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubeflow_pipelines/4180/kubeflow-pipeline-frontend-test/1294002360783212544#1:build-log.txt%3A51

Bobgy

Thank you! all looking good at high level
/lgtm

Read through code thoroughly and only left some nit picking

Bobgy · 2020-10-10T10:09:16Z

frontend/src/lib/Utils.tsx

+export async function decodeCompressedNodes(compressedNodes: string): Promise<object> {
+  return new Promise<object>(resolve => {
+    const compressedBuffer = Buffer.from(compressedNodes, 'base64');
+    zlib.gunzip(compressedBuffer, { windowBits: 15 }, (error, result: Buffer) => {


nit: what does the windowBits: 15 mean?

Can we avoid the magic number?

Sorry forgot to remove it, fixed here: e93a060

Bobgy · 2020-10-10T10:10:45Z

frontend/src/lib/Utils.tsx

+    zlib.gunzip(compressedBuffer, { windowBits: 15 }, (error, result: Buffer) => {
+      if (error) {
+        logger.error('failed to gunzip data ', error);
+        resolve(undefined);


nit: a more common practice is to reject when there's an error, but it's up to you whether you want to change

You're right, I will reject here and catch error in RunDetails.

Done in 5b46a4d

Bobgy · 2020-10-10T10:15:07Z

frontend/src/pages/RunDetails.test.tsx

@@ -167,6 +168,9 @@ describe('RunDetails', () => {
    // Hide expected warning messages
    warnSpy = jest.spyOn(Utils.logger, 'warn').mockImplementation();

+    // Mock decodeCompressedNodes to avoid dequeuing chained promises
+    decodeCompressedNodesSpy = jest.spyOn(Utils, 'decodeCompressedNodes').mockImplementation();


nit: decodeCompressedNodes itself is very important to test in this test case, because otherwise we are not verifying this feature end to end.

I understand you may be blocked by sth. You can put a TODO comment here too if you cannot figure out by yourself.

If you'd want to try testing it. Here are my ideas:
I'm not sure how zlib will trigger the callback, I guess if simply await is not enough, we can use setTimeout to explicitly wait some long time period so zlib can decompress.

Well, I already unit tested decodeCompressedNodes in Utils.test.tsx.
The setTimeout trick will work for sure but not so elegant.

I test the wait and it works, thanks for the hint: 8491c5c

Bobgy · 2020-10-10T10:15:50Z

frontend/src/pages/RunDetails.test.tsx

+    expect(getByTestId('graph')).toMatchInlineSnapshot(`
+      <pre
+        data-testid="graph"
+      >
+        Node node1
+        Node node1-running-placeholder
+        Edge node1 to node1-running-placeholder
+      </pre>
+    `);


nice! now it's showing readable content

axelborja · 2020-10-12T14:23:16Z

Thanks @radcheb for this fix !! 👏

…ch error on run load

radcheb · 2020-10-15T09:31:44Z

@Bobgy could you review and merge if it's ok ?

Bobgy · 2020-10-15T13:11:22Z

@Bobgy could you review and merge if it's ok ?

@radcheb Sure, I'll review tomorrow.

Bobgy

Thank you @radcheb for your perseverance!
This looks awesome!

There's one last minor mistake to fix, otherwise all good!
/lgtm

Bobgy · 2020-10-16T05:41:02Z

frontend/src/pages/RunDetails.test.tsx

+    await new Promise(resolve => setTimeout(resolve, 500));
+    jest.useFakeTimers();
+
+    expect(tree).toMatchSnapshot('');


this should be

expect(getByTestId('graph')).toMatchInlineSnapshot()

actually, I'm not sure whether graph will actually render, if not you can also just assert something like

expect(getAllByTestId('graph')).toEqual([])

Yes in this case the graph should be empty, I will use your second suggestion.

Well getAllByTestId didn't work since it requires at least one element in the list, it was rather queryAllByTestId who did the job.

Bobgy · 2020-10-16T14:26:02Z

/lgtm
/approve

Really valuable fix!
@racheb Thank you for your contribution!

k8s-ci-robot · 2020-10-16T14:26:11Z

[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

radcheb · 2020-10-16T14:38:03Z

Thanks @Bobgy for accepting and merging. Will this be included in a future release ?

Bobgy · 2020-10-16T14:54:32Z

It's not clear whether we will release 1.0.4 or not. If not, it will come in 1.1.0

…ubeflow#4180) * frontend: add large pipeline example to mocked data * add util function decodeCompressedNodes * decode compressedNodes in workflow if node is empty and compressedNodes exit * fix decodeCompressedNodes * prettify code * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * add util function decodeCompressedNodes * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * reformat code * update jest snapshot to add compressed node case * fix tests * reformat code * update RunDetails snapshot * remove dupplicate test code * RunDetails: remove compressedNodes after decoding it * reformat decodeCompressedNodes + add failure case test * decodeCompressedNodes returns undefined on error * RunDetails decodeCompressedNodes test: snapshot same as single node graph * do not wait for decodeCompressedNodes + debug print workflow * fix Run load + refresh snapshot * format code * Fix one-node compressed workflow graph + update snapshot * reformat code * rename large pipeline name * fix decompressNodes to work in browser * fix test * fix tests * remove some of the console.log * clean code * address comments * address comments: wait for zlib instead of mocking decodeCompressedNodes * address comments: decodeCompressedNodes reject in case of error + catch error on run load * address comments

* frontend: add large pipeline example to mocked data * add util function decodeCompressedNodes * decode compressedNodes in workflow if node is empty and compressedNodes exit * fix decodeCompressedNodes * prettify code * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * add util function decodeCompressedNodes * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * reformat code * update jest snapshot to add compressed node case * fix tests * reformat code * update RunDetails snapshot * remove dupplicate test code * RunDetails: remove compressedNodes after decoding it * reformat decodeCompressedNodes + add failure case test * decodeCompressedNodes returns undefined on error * RunDetails decodeCompressedNodes test: snapshot same as single node graph * do not wait for decodeCompressedNodes + debug print workflow * fix Run load + refresh snapshot * format code * Fix one-node compressed workflow graph + update snapshot * reformat code * rename large pipeline name * fix decompressNodes to work in browser * fix test * fix tests * remove some of the console.log * clean code * address comments * address comments: wait for zlib instead of mocking decodeCompressedNodes * address comments: decodeCompressedNodes reject in case of error + catch error on run load * address comments

Bobgy · 2020-10-23T06:16:39Z

Hi @radcheb, we just released 1.0.4 with your fix!

axelborja · 2020-10-27T08:55:19Z

Thanks 🙏

radcheb · 2020-10-27T14:46:56Z

Hi @radcheb, we just released 1.0.4 with your fix!

Thanks @Bobgy for your assistance and thus this great news.

…ubeflow#4180) * frontend: add large pipeline example to mocked data * add util function decodeCompressedNodes * decode compressedNodes in workflow if node is empty and compressedNodes exit * fix decodeCompressedNodes * prettify code * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * add util function decodeCompressedNodes * Frontend:test Utils decodeCompressedNodes * test RunDetails page render compressed grah * reformat code * update jest snapshot to add compressed node case * fix tests * reformat code * update RunDetails snapshot * remove dupplicate test code * RunDetails: remove compressedNodes after decoding it * reformat decodeCompressedNodes + add failure case test * decodeCompressedNodes returns undefined on error * RunDetails decodeCompressedNodes test: snapshot same as single node graph * do not wait for decodeCompressedNodes + debug print workflow * fix Run load + refresh snapshot * format code * Fix one-node compressed workflow graph + update snapshot * reformat code * rename large pipeline name * fix decompressNodes to work in browser * fix test * fix tests * remove some of the console.log * clean code * address comments * address comments: wait for zlib instead of mocking decodeCompressedNodes * address comments: decodeCompressedNodes reject in case of error + catch error on run load * address comments

k8s-ci-robot requested review from Bobgy and IronPan July 8, 2020 19:18

k8s-ci-robot added the size/XXL label Jul 8, 2020

googlebot added the cla: no label Jul 8, 2020

k8s-ci-robot added the needs-ok-to-test label Jul 8, 2020

radcheb mentioned this pull request Jul 8, 2020

Frontend UI cannot display large workflows #4179

Closed

googlebot added cla: yes and removed cla: no labels Jul 8, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jul 8, 2020

Bobgy reviewed Jul 16, 2020

View reviewed changes

frontend/src/pages/RunDetails.tsx Show resolved Hide resolved

k8s-ci-robot assigned Bobgy Jul 16, 2020

k8s-ci-robot added the lgtm label Jul 16, 2020

Bobgy added the cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch label Jul 16, 2020

k8s-ci-robot removed the lgtm label Aug 9, 2020

radcheb force-pushed the frontend-fix-parse-large-workflow branch from dba46f4 to f63c7a3 Compare August 9, 2020 16:57

radcheb requested a review from Bobgy August 9, 2020 19:56

Bobgy reviewed Oct 10, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Oct 10, 2020

address comments

e93a060

k8s-ci-robot removed the lgtm label Oct 12, 2020

radcheb added 2 commits October 13, 2020 16:08

address comments: wait for zlib instead of mocking decodeCompressedNodes

8491c5c

address comments: decodeCompressedNodes reject in case of error + cat…

5b46a4d

…ch error on run load

radcheb requested a review from Bobgy October 13, 2020 15:47

Bobgy suggested changes Oct 16, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Oct 16, 2020

address comments

0415d6b

k8s-ci-robot removed the lgtm label Oct 16, 2020

k8s-ci-robot added the lgtm label Oct 16, 2020

k8s-ci-robot added the approved label Oct 16, 2020

Bobgy approved these changes Oct 16, 2020

View reviewed changes

k8s-ci-robot merged commit 02b0899 into kubeflow:master Oct 16, 2020

Bobgy added the cherrypicked cherry picked to release branch `release-x.y` label Oct 22, 2020

fix(frontend): fix parsing large workflow graph. Fixes #4179 #4180

fix(frontend): fix parsing large workflow graph. Fixes #4179 #4180

Conversation

radcheb commented Jul 8, 2020 • edited Loading

googlebot commented Jul 8, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented Jul 8, 2020

radcheb commented Jul 8, 2020

googlebot commented Jul 8, 2020

kubeflow-bot commented Jul 8, 2020

Ark-kun commented Jul 8, 2020

Ark-kun commented Jul 8, 2020

Bobgy left a comment

Choose a reason for hiding this comment

radcheb commented Aug 9, 2020

Bobgy commented Aug 9, 2020

radcheb commented Aug 10, 2020

radcheb commented Aug 13, 2020

radcheb commented Aug 13, 2020

Bobgy commented Aug 14, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radcheb Oct 12, 2020 • edited Loading

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

axelborja commented Oct 12, 2020

radcheb commented Oct 15, 2020

Bobgy commented Oct 15, 2020

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

Bobgy commented Oct 16, 2020

k8s-ci-robot commented Oct 16, 2020

radcheb commented Oct 16, 2020

Bobgy commented Oct 16, 2020

Bobgy commented Oct 23, 2020

axelborja commented Oct 27, 2020

radcheb commented Oct 27, 2020

radcheb commented Jul 8, 2020 •

edited

Loading

radcheb Oct 12, 2020 •

edited

Loading