[BEAM-10545] KernelModel and jest tests #12372

KevinGG · 2020-07-25T00:23:28Z

Added a KernelModel module that handles messaging between the
frontend and the connected kernel. The model silently executes code in the kernel and handles IOPub responses from the kernel for information of the current interactive environment. UI components and other data models can use this module as a data model to communicate with the kernel. When instantiated, this model does not have to be a singleton.
Integrated jest test framework with typescript.
Advanced prettier to 1.19 so that it can parse typescript optional
chaining syntax.
The tests can be executed with jlpm jest. Test details see README.
Changes made by jlpm are:
jlpm add --dev jest @types/jest ts-jest identity-obj-proxy
jlpm upgrade prettier@^1.19.0

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

KevinGG · 2020-07-25T00:33:55Z

yarn.lock is auto-generated by jlpm (the yarn came with JupyterLab), there is no need to review that file.

KevinGG · 2020-07-26T02:18:56Z

Run Python2_PVR_Flink PreCommit

1. Added a KernelModel module that handles messaging between the frontend and the connected kernel. 2. Integrated jest test framework with typescript. 3. Advanced prettier to 1.19 so that it can parse typescript optional chaining syntax. 4. The tests can be executed with `jlpm jest`. Test details see README. 5. Changes made by jlpm are: jlpm add --dev jest @types/jest ts-jest identity-obj-proxy jlpm upgrade prettier@^1.19.0

KevinGG · 2020-07-27T20:51:33Z

Rebased to resolve merge conflicts.

KevinGG · 2020-07-27T21:42:29Z

R: @prodonjs
PTAL, thanks!

prodonjs

No concerns just one small style nit.

prodonjs · 2020-07-29T18:53:04Z

...am/runners/interactive/extensions/apache-beam-jupyterlab-sidepanel/src/kernel/KernelModel.ts

+    });
+  }
+
+  private _onIOPub = (msg: KernelMessage.IIOPubMessage): void => {


This is just a nit, but for consistency, I would keep the private method signature the same as the public methods (ie. private _onIOPub(msg: KernelMessage.IIOPubMessage) {...) as opposed to this syntax which assigns the lambda function to the private data member.

Thanks, Jason! I've made the change to the private field. Additionally, it requires a bind(this) when assigning the function to the future.

…ad of arrow function assignment. This requires binding `this` when assigning the function to the `future`.

prodonjs

LGTM

KevinGG · 2020-07-29T20:04:25Z

Run Python2_PVR_Flink PreCommit

KevinGG · 2020-07-29T20:48:20Z

R: @aaltay
PTAL, thx!

aaltay

LGTM. I do not believe I am a good typescript reviewer. It would be good if you can ask on dev@ and see if there could be better reviewers for future PRs.

aaltay

Retracting my LGTM, I have some questions.

And a high level question, what is the purpose of this module? (Could you add more details to what you wrote as part of the PR description?)

aaltay · 2020-07-30T17:44:02Z

...ractive/extensions/apache-beam-jupyterlab-sidepanel/src/__tests__/kernel/KernelModel.test.ts

+    output_type: 'display_data',
+    data: {
+      'text/html': '<div></div>',
+      'application/javascript': 'console.log(1);'


What does this console.log's do? Are they executed?

It's a valid Javascript statement that logs to the console.

In the test, if it is executed, the result shows up in the terminal.
This unit test doesn't do the execution though because it's just a data model not a UI component.

aaltay · 2020-07-30T17:46:47Z

...am/runners/interactive/extensions/apache-beam-jupyterlab-sidepanel/src/kernel/KernelModel.ts

+      if (dataInPlainText) {
+        try {
+          // The slice removes trailing single quotes from the nbformat output.
+          // The replace removes literal backslashes from the nbformat output.


it seems to replace a backslash and a quote? \\'

It's removing characters in nbformat texts.

Kernel generates json.
When messaged to frontend by Jupyter in nbformat, the text becomes 'json_with_backslashes'.

This removes the "'" from both ends of the string.
Then removes all the backlashes.

The result text will be a valid json that is parsable.

aaltay · 2020-07-30T17:48:07Z

...am/runners/interactive/extensions/apache-beam-jupyterlab-sidepanel/src/kernel/KernelModel.ts

+    }
+    this.future = this._sessionContext.session?.kernel?.requestExecute({
+      code: KernelCode.COMMON_KERNEL_IMPORTS + code,
+      silent: !expectReply,


If silent is the name of the underlying api, why do we use an oppositely behaving expectReply flag?

The problem with silent is that it's confusing with the existence of store_history.

Intuitively, if silent, the kernel execution should not store history. However, it's not that case.

silent only means if the execution should return a result. The history is stored. The side effects are obvious.

So you will see execution_count increased even when you set silent to true.

Facading it with expectReply makes it clear when using the model.

aaltay · 2020-07-30T17:50:21Z

...ractive/extensions/apache-beam-jupyterlab-sidepanel/src/__tests__/kernel/KernelModel.test.ts

+  expect(kernelModel.displayData).toEqual([displayData]);
+});
+
+it('handles display update from IOPub channel', () => {


what is the difference between display data and display update messages?

Think of display data as incrementally displaying contents while display update as modifying contents already displayed.

An example,

display data is [1, 2 ,3], you'll see 1 2 3 being displayed;

display update is [1, 2, 3] for a same display_id, you'll see 1, then it becomes 2, then it becomes 3.

KevinGG · 2020-07-30T18:29:13Z

Retracting my LGTM, I have some questions.

And a high level question, what is the purpose of this module? (Could you add more details to what you wrote as part of the PR description?)

The purpose of this module is to allow the extension to execute code in the kernel silently to retrieve information of current interactive environment, then handles IOPub messages sent back from the kernel and allow other components to use this module as a data model.

KevinGG · 2020-07-30T18:33:17Z

LGTM. I do not believe I am a good typescript reviewer. It would be good if you can ask on dev@ and see if there could be better reviewers for future PRs.

Thanks! For typescript review, Jason has agreed to take a look at my future PRs. He is the TL of CAIP notebooks and works on other extensions: https://github.com/GoogleCloudPlatform/jupyter-extensions.
I'll still need your and other committer's feedback as how the code fits in the Beam repo and how others can contribute to it.

probot-autolabeler bot added the python label Jul 25, 2020

KevinGG force-pushed the BEAM-10545 branch from f9f5dd0 to b05204e Compare July 27, 2020 20:51

prodonjs reviewed Jul 29, 2020

View reviewed changes

Change the syntax of private _onIOPub to a function declaration inste…

8f24efa

…ad of arrow function assignment. This requires binding `this` when assigning the function to the `future`.

prodonjs approved these changes Jul 29, 2020

View reviewed changes

aaltay approved these changes Jul 30, 2020

View reviewed changes

aaltay reviewed Jul 30, 2020

View reviewed changes

KevinGG requested a review from aaltay July 30, 2020 18:49

aaltay approved these changes Jul 30, 2020

View reviewed changes

aaltay merged commit c64cf34 into apache:master Jul 30, 2020

KevinGG deleted the BEAM-10545 branch July 31, 2020 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-10545] KernelModel and jest tests #12372

[BEAM-10545] KernelModel and jest tests #12372

KevinGG commented Jul 25, 2020 •

edited

KevinGG commented Jul 25, 2020

KevinGG commented Jul 26, 2020

KevinGG commented Jul 27, 2020

KevinGG commented Jul 27, 2020

prodonjs left a comment

prodonjs Jul 29, 2020

KevinGG Jul 29, 2020

prodonjs left a comment

KevinGG commented Jul 29, 2020

KevinGG commented Jul 29, 2020

aaltay left a comment

aaltay left a comment

aaltay Jul 30, 2020

KevinGG Jul 30, 2020 •

edited

aaltay Jul 30, 2020

KevinGG Jul 30, 2020

aaltay Jul 30, 2020

KevinGG Jul 30, 2020

aaltay Jul 30, 2020

KevinGG Jul 30, 2020

KevinGG commented Jul 30, 2020

KevinGG commented Jul 30, 2020

[BEAM-10545] KernelModel and jest tests #12372

[BEAM-10545] KernelModel and jest tests #12372

Conversation

KevinGG commented Jul 25, 2020 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

KevinGG commented Jul 25, 2020

KevinGG commented Jul 26, 2020

KevinGG commented Jul 27, 2020

KevinGG commented Jul 27, 2020

prodonjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prodonjs left a comment

Choose a reason for hiding this comment

KevinGG commented Jul 29, 2020

KevinGG commented Jul 29, 2020

aaltay left a comment

Choose a reason for hiding this comment

aaltay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGG Jul 30, 2020 • edited

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

KevinGG commented Jul 30, 2020

KevinGG commented Jul 30, 2020

KevinGG commented Jul 25, 2020 •

edited

KevinGG Jul 30, 2020 •

edited