DATAUP-176 slow startup #1840

briehl · 2020-10-05T06:51:31Z

Description of PR purpose/changes

This PR addresses some of the events that occur during Narrative startup. The startup process consists of 5 primary steps that are checked off of a tracking div, ending with the disappearance of that div.

There are two of these that are closely dependent on the kernel and what it's doing - these are checkbox 4 and 5 on the list. In the first kernel step, we try to make sure the connection between the FE and the Jupyter notebook kernel exists and is stable and ready for input. This is currently done by waiting for the kernel_ready.Kernel event to get fired. This happens when the kernel is connected and idle. This can be a problem if the kernel already exists and is busy, which can happen on a page reload. It's therefore possible for the kernel_ready.Kernel event to not be fired in any reasonable amount of time. It's equally likely that the kernel isn't really usable at that point, and we're best off preventing the user from getting frustrated with that, but at that point there might be other interventions. Either way, this shouldn't block a user from getting to their Narrative.

Setting that event to kernel_connected.Kernel addresses this by filling the checkbox as soon as the kernel is connected to the FE. It might not be ready for events at that point, but this shouldn't affect the user experience that much, especially if the user is expecting some cell to finish running.

This PR also starts the process of cleaning up and testing the kbaseNarrative module. I'm going to try to do that in chunks, since it's a huge module.

Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

Jira Ticket / Issue

e.g. https://kbase-jira.atlassian.net/browse/DATAUP-X

Added the Jira Ticket to the title of the PR e.g. (DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass in travis and locally
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

My code follows the guidelines at https://sites.google.com/truss.works/kbasetruss/development
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have run Black and Flake8 on changed Python Code manually or with git precommit (and the travis build passes)

Updating Version and Release Notes (if applicable)

Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

…and additional tests

coveralls · 2020-10-05T07:01:46Z

Coverage increased (+0.8%) to 17.167% when pulling a37e97a on DATAUP-176-slow-startup into 4953ad1 on develop.

briehl · 2020-10-06T17:13:31Z

This PR will also probably lower the coverage a little bit, since some JS files were missing from that calculation. Sigh. Will get updated in a separate PR.

briehl · 2020-10-07T23:06:43Z

kbase-extension/static/kbase/js/kbaseNarrative.js

-    // This should not be run until AFTER the notebook has been loaded!
-    // It depends on elements of the Notebook metadata.
-    Narrative.prototype.init = function () {
+    Narrative.prototype.init = function (jobsReadyCallback) {


This function has most of the main changes that aren't just linting / whitespace shifts.

jobsReadyCallback was put in to give the test suite something to hang a Promise onto, since the main tasks here are done by callback to DOM / jquery events.

I should have scrolled down to here and ignored the above! 😖

briehl · 2020-10-07T23:08:15Z

kbase-extension/static/kbase/js/kbaseNarrative.js

        };

-        $([Jupyter.events]).on('notebook_loaded.Notebook', function () {
+        $([Jupyter.events]).on('notebook_loaded.Notebook', () => {


Everything under this event is still untested, and probably warrants a little rewriting. Not the focus of this PR, but will be in the next one.

briehl · 2020-10-07T23:09:50Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+                .then(() => this.narrController.render())
+                .finally(() => this.sidePanel.render());
+        });
+        $([Jupyter.events]).on('kernel_connected.Kernel', () => {


Here's the primary change - kernel_ready.Kernel only fires when the kernel is ready for input, and not busy. If there's something running, it'll never fire. This will look like there's a failure, when really the kernel is busily crunching some code.

kernel_connected.Kernel fires when the connection is set up properly. It should be much more consistent here.

I also made the event response set up in parallel with the notebook_loaded.Notebook event above, instead of only set up when the Notebook event fires. I've never seen them not go in that order, but it's possible, and this is cleaner.

briehl · 2020-10-07T23:11:53Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+                .initCommChannel()
+                .then(() => {
+                    this.loadingWidget.updateProgress('jobs', true);
+                    if (jobsReadyCallback) {


The callback should be optional - it's mainly used for testing.

briehl · 2020-10-07T23:12:55Z

kbase-extension/static/kbase/js/kbaseNarrative.js

-    // This should not be run until AFTER the notebook has been loaded!
-    // It depends on elements of the Notebook metadata.
-    Narrative.prototype.init = function () {
+    Narrative.prototype.init = function (jobsReadyCallback) {
        // NAR-271 - Firefox needs to be told where the top of the page is. :P
        window.scrollTo(0, 0);


This can probably be removed by now, but that can get tested in a proceeding PR.

briehl · 2020-10-07T23:13:16Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+        * When that event is no longer bound (i.e. when the node is removed, OR when .unbind is called)
+        * it triggers the 'remove' function. Lets us keep track of when widgets get removed
+        * in the registerWidget function below.
+        */
        $.event.special.destroyed = {


I don't think this ever gets invoked, and can probably be removed as well. A later PR.

briehl · 2020-10-07T23:17:09Z

test/unit/spec/kbaseNarrativeSpec.js

+    describe('Test the kbaseNarrative module', () => {
+        let loginDiv = $('<div>');
+
+        beforeEach(async () => {


Testing was challenging.

beforeEach mocks up the /token and /me calls to auth - these define the auth session, which gets consumed by Narrative.init. So they're kinda necessary. The mock returns a truncated object with enough info to get through.

It also mocks a "Jupyter" object. We aren't testing the Jupyter machinery, but need some of it - specifically the kernel - to be present. Each of those elements are either called by the Narrative.init object, or the jobCommChannel. Mocking the comm channel is probably a better solution, but due to the nature of AMD load orders and testing, that's not trivial. This is verbose, but much easier to control with few side effects.

briehl · 2020-10-07T23:17:46Z

test/unit/spec/kbaseNarrativeSpec.js

+
+        it('Should instantiate', () => {
+            const narr = new Narrative();
+            expect(narr.maxNarrativeSize).toBe('10 MB');


Just using this magic value to make sure there's a Narrative object present.

briehl · 2020-10-07T23:18:19Z

test/unit/spec/kbaseNarrativeSpec.js

+
+        it('init should fail as expected when the job connection fails', () => {
+            return new Promise((resolve, reject) => {
+                Jupyter.notebook.kernel.comm_info = () => {


tweak the mocked kernel to always throw an error on connection.

briehl · 2020-10-07T23:18:59Z

test/unit/spec/kbaseNarrativeSpec.js

+            const narr = new Narrative();
+            expect(narr.getAuthToken()).toBe(FAKE_TOKEN);
+        });
+


There's a zillion more tests for that module, but I wanted to keep the code in this PR more manageable. More upcoming!

eamahanna

LGTM. This looks like a beast to test. Thanks for tackling it and including notes to help reviewers understand what was happening.

Tests are failing

briehl · 2020-10-20T23:57:03Z

@eamahanna Tests are passing now. I'm not sure why they failed before. :P

ialarmedalien · 2020-10-23T21:40:22Z

test/unit/spec/kbaseNarrativeSpec.js

+
+        it('Should return a boolean for is_loaded', () => {
+            const narr = new Narrative();
+            expect(narr.isLoaded()).toEqual(DEFAULT_FULLY_LOADED);  // default as set in


// default as set in

unfinished comment?

I choose to have it be dangling. Where could it be set!? I want the reader to have the same anxiety I feel when I run these tests.

(ok, I'll fix it. Thanks for catching.)

ialarmedalien · 2020-10-23T21:49:55Z

test/unit/spec/kbaseNarrativeSpec.js

+            expect(narr.uiModeIs('edit')).toBe(DEFAULT_WRITABLE);
+            expect(narr.uiModeIs('view')).toBe(!DEFAULT_WRITABLE);
+            Jupyter.notebook.writable = !Jupyter.notebook.writable;
+            expect(narr.uiModeIs('edit')).toBe(!DEFAULT_WRITABLE);
+            expect(narr.uiModeIs('view')).toBe(DEFAULT_WRITABLE);


Might also be worth checking that Jupyter.notebook.writable and DEFAULT_WRITABLE are the same when you first instantiate the new Narrative object, so that you know that the initialisation has worked correctly and that the uiModeIs function is returning the correct result.

That's a very good idea.

ialarmedalien · 2020-10-23T21:52:13Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+            } else {
+                return new Workspace(Config.url('workspace'), {
+                    token: this.getAuthToken(),
+                })
+                    .get_workspace_info({ id: this.workspaceId })


can remove a level of indenting here as the if statement triggers a return.

ialarmedalien · 2020-10-23T21:53:15Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+        })
+            .get_workspace_info({ id: this.workspaceId })


the indent here is a little confusing! presumably that's prettier at work?

I agree! That's good ol' eslint trying its best. :) I'll rework to make, er, prettier.

ialarmedalien · 2020-10-23T21:54:47Z

kbase-extension/static/kbase/js/kbaseNarrative.js

            } catch (ex) {
-                // console.warn('Error removing shortcut "'  + shortcut +'"', ex);
+                // console.warn('Error removing shortcut ''  + shortcut +'"', ex);


this line won't parse correctly if uncommented

This is pre-existing code, I know, but do you know why has this one been commented out but the others left in?

I just tested, and it's commented out because of how that whole block is structured. It tries to remove those shortcuts from both the command and edit lists, but they're only in one or the other, so there's 30ish cases of warning spam on every page load if that's uncommented.

That whole function needs an overhaul, imo. Next PR.

ialarmedalien · 2020-10-23T21:57:27Z

kbase-extension/static/kbase/js/kbaseNarrative.js

+        $([Jupyter.events]).on(
+            'delete.Cell',
+            function () {
+                // this.enableKeyboardManager();
+            }.bind(this)
+        );


still needed?

ialarmedalien · 2020-10-23T21:58:38Z

kbase-extension/static/kbase/js/kbaseNarrative.js

@@ -322,35 +349,47 @@ define([
     * after there's a visible DOM element for it to render in.
     */
    Narrative.prototype.initSharePanel = function () {
-        var sharePanel = $('<div style="text-align:center"><br><br><img src="' +
+        var sharePanel = $(
+                '<div style="text-align:center"><br><br><img src="' +


maybe in another PR that inline style and the <br>s could be removed and replaced by a class with an appropriate amount of padding-top?

ialarmedalien

The core of the code looks fine. Just a few comments that you may or may wish to act upon.

briehl · 2020-10-23T22:19:27Z

The core of the code looks fine. Just a few comments that you may or may wish to act upon.

Thanks. Yeah, I'll address those. There's a TON of other changes I'd like to make to that module in upcoming PRs (probably in some free time, if I can wrangle some). I'd first like to get much more test coverage, then start peeling out old code that isn't used, and update some old style code to be more in-line with choices we've made recently.

sonarcloud · 2020-10-23T23:10:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

briehl · 2020-10-23T23:39:47Z

Once tests pass, I think I can merge. That might not be until Monday, though, with Travis's backlog...

briehl · 2020-10-23T23:43:07Z

Noting here that this is going against develop, which should be periodically merged into truss anyway.

briehl added 2 commits September 29, 2020 14:23

add different event to startup

0f0a823

add a different event for detecting kernel connections, some cleanup …

0b0b9a5

…and additional tests

briehl added 3 commits October 5, 2020 15:03

fix linting of kbaseNarrative.js

82ddc5b

add missing files to coverage

f380cf5

got tests working, now to actually tests what we want

7f517e1

briehl added 5 commits October 7, 2020 13:57

got init happy test working

6014420

fix tests

2165fc3

fix tests to make codacy happy

d640baa

update comments

c09cacc

make jobsReadyCallback optional

e11daec

briehl marked this pull request as ready for review October 7, 2020 23:05

briehl commented Oct 7, 2020

View reviewed changes

briehl requested review from bio-boris, eamahanna and ialarmedalien October 7, 2020 23:19

eamahanna previously approved these changes Oct 8, 2020

View reviewed changes

ialarmedalien reviewed Oct 23, 2020

View reviewed changes

ialarmedalien approved these changes Oct 23, 2020

View reviewed changes

fix some minor things in the tests

9a9fded

briehl added 2 commits October 23, 2020 15:22

remove unused event listener

76355e4

minor visual cleanup

a37e97a

briehl merged commit ad31ace into develop Oct 26, 2020

briehl deleted the DATAUP-176-slow-startup branch October 26, 2020 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAUP-176 slow startup #1840

DATAUP-176 slow startup #1840

briehl commented Oct 5, 2020

coveralls commented Oct 5, 2020 •

edited

Loading

briehl commented Oct 6, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

ialarmedalien Oct 23, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

briehl Oct 7, 2020

eamahanna left a comment

briehl commented Oct 20, 2020

ialarmedalien Oct 23, 2020

briehl Oct 23, 2020

ialarmedalien Oct 23, 2020

briehl Oct 23, 2020

ialarmedalien Oct 23, 2020

ialarmedalien Oct 23, 2020

briehl Oct 23, 2020

ialarmedalien Oct 23, 2020

ialarmedalien Oct 23, 2020

briehl Oct 23, 2020

ialarmedalien Oct 23, 2020

ialarmedalien Oct 23, 2020 •

edited

Loading

ialarmedalien left a comment

briehl commented Oct 23, 2020

sonarcloud bot commented Oct 23, 2020

briehl commented Oct 23, 2020

briehl commented Oct 23, 2020

DATAUP-176 slow startup #1840

DATAUP-176 slow startup #1840

Conversation

briehl commented Oct 5, 2020

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

coveralls commented Oct 5, 2020 • edited Loading

briehl commented Oct 6, 2020

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

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

eamahanna left a comment

Choose a reason for hiding this comment

briehl commented Oct 20, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

ialarmedalien left a comment

Choose a reason for hiding this comment

briehl commented Oct 23, 2020

sonarcloud bot commented Oct 23, 2020

briehl commented Oct 23, 2020

briehl commented Oct 23, 2020

coveralls commented Oct 5, 2020 •

edited

Loading

ialarmedalien Oct 23, 2020 •

edited

Loading