Update Plugin API [v2] #1179

nikitakit · 2018-01-05T01:33:55Z

The goal of these changes is to make the plugin API more generally useful, by allowing plugins to intercept or issue kernel commands.

Here are just some of the plugin ideas that these changes aim to make possible:

A plugin that fixes indentation by removing common leading whitespace (see Request: strip _common_ leading whitespace from input before passing to kernel #862). This is useful if you'd like to highlight and run some code that's inside a function, meaning that it's indented by some amount.
Moving watches behavior into a plugin
Remapping executing expr? to opening the inspector for expr, for those of us with lots of muscle memory from using notebooks

An example of how to use the updated API is currently in the diff for lib/main.js (actual location for examples is TBD).

These changes have similar goals to my previous approach, but I'm now basing them on the concept of middleware classes rather than event handlers. I think this is a more hassle-free approach.

Note that I'm enforcing separation between the plugin interface and hydrogen internals: in particular, the way to extend hydrogen does not involve inheriting from any classes defined in hydrogen core. This introduces an extra level of indirection, but I think it's the right approach in the long run because it allows hydrogen core to evolve at a different pace from the plugin API. It also allows multiple versions of the plugin API to coexist, which I think is good practice for maintaining backwards compatibility.

The WIP tag reflects the fact that there are still two unresolved questions that I'd like feedback on (assuming the overall approach is sound).

~~Figuring out the precise syntax for defining middleware (options are presented at hydrogen-kernel.js line 73)~~

nikitakit · 2018-01-05T01:40:02Z

package.json

@@ -90,7 +90,8 @@
    },
    "hydrogen.provider": {
      "versions": {
-        "1.1.0": "provideHydrogen"
+        "1.1.0": "provideHydrogen",
+        "1.2.0": "provideHydrogen"


Note that keeping around multiple versions is intentional. They can still point to the same place because the new version is a strict superset of the old one.

nikitakit · 2018-01-05T01:53:54Z

lib/kernel.js

+  execute: (code: string, callWatches: boolean, onResults: Function) => void;
+  complete: (code: string, onResults: Function) => void;
+  inspect: (code: string, cursorPos: number, onResults: Function) => void;
+}


If you think that callbacks are too old-fashioned, it may be possible to convert these to async syntax. Note however that onResults gets called multiple times, so this will rely on async iterator features that are quite recent.

I like the idea of moving to async and promises as much as possible. Though I don't know if it'd be worth the effort in that case.

I'd rather prefer us moving to @nteract/messaging, enchannel-zmq-backend and rx-jupyter using RxJS which provides excellent primitives for handling streaming data.

Alternatively we could rely more on MobX for these kind of things.

nikitakit · 2018-01-05T04:23:09Z

lib/kernel.js

@@ -74,31 +99,58 @@ export default class Kernel {
  }

  interrupt() {
-    throw new Error("Kernel: interrupt method not implemented");
+    this.rawInterrupt();


This should call the middleware. I'll fix that in the next patchset.

lgeiger

@nikitakit Thanks for continuing the effort in expanding our plugin API. I like the idea of using a middleware to interact with Hydrogen. Though I don't have any experience with designing rugged APIs for other people to use 😄

I'm happy to merge this soon to keep us moving forward. Since there are not many plugins using this API we can iterate on it and see how it feels when using it for real.

@rgbkrk @BenRussert I'd like to have your input on what you think about this approach.

lgeiger · 2018-01-07T16:00:20Z

lib/kernel.js

+    if (restart === true) {
+      this.middleware.shutdown(restart, onRestarted);
+    } else {
+      this.middleware.shutdown(false);


I'd prefer to keep shutdown and restart separate methods. The restart argument is coming from the jupyter messaging protocol and doesn't actually restart the kernel.

I see that there's a restart argument in the messaging protocol, but does that argument even serve a useful purpose that hydrogen cares about?

If not, maybe it would still make sense to combine restart and shutdown in the middleware? By this I mean that restart: ?boolean will differentiate between true restart/shutdown rather than controlling the messaging protocol argument.

Actually what I said above is already the case in the code: setting restart=true here will initiate a true restart, not just toggle a flag in the shutdown message.

I'm not sure that exposing this restart flag to plugins would be useful in any way. (That said, the case can be made that separating shutdown/restart middleware will avoid needless confusion.)

lgeiger · 2018-01-07T16:08:37Z

lib/plugin-api/hydrogen-kernel.js

+  // yet another alternative would be:
+  //    kernel.addMiddleware({
+  //      execute: (next, code, callWatches, onResults) => {
+  //        next.execute(code, callWatches, onResults);


I like the functional approach 👍

lgeiger · 2018-01-07T16:17:11Z

lib/kernel.js

+  execute: (code: string, callWatches: boolean, onResults: Function) => void;
+  complete: (code: string, onResults: Function) => void;
+  inspect: (code: string, cursorPos: number, onResults: Function) => void;
+}


I like the idea of moving to async and promises as much as possible. Though I don't know if it'd be worth the effort in that case.

I'd rather prefer us moving to @nteract/messaging, enchannel-zmq-backend and rx-jupyter using RxJS which provides excellent primitives for handling streaming data.

Alternatively we could rely more on MobX for these kind of things.

nikitakit · 2018-01-08T19:57:50Z

Thanks for taking a look at this!

I did a quick round of updates to switch away from classes. In terms of using async code instead of result callbacks, I've decided that there are too many details to do this right now. Later on if plugins are using this we can figure out something based on actual usage patterns.

Also I think I now have an idea of how to handle result objects in the API (though this is not implemented yet). Currently hydrogen takes objects formatted according to the jupyter message protocol, converts them to custom result objects, and then calls onResults. What I'd like to do instead is have the raw jupyter messages pass through the middleware, and only be converted to a hydrogen-internal format afterwards. That way if plugins want to interact with the results stream, they can work with something that's in a well-documented format.

BenRussert · 2018-01-11T03:58:32Z

I like this approach a lot!

Some docs to explain some usage would help. I think the example from main.js above could be moved to an example plugin (sometimes even more helpful than docs)

nikitakit · 2018-01-12T05:59:33Z

I just pushed an update that ensures that middleware onResults callbacks always receive messages defined under the Jupyter messaging spec, and not hydrogen-internal types. This is a pretty large diff so I'd appreciate any code review and testing this can get!

With this change, it should be possible for plugins to modify replies that kernels send (for example, to inject their own messages into the result bubbles). Even though the middleware now operates on Jupyter messages, the ZMQKernel/WSKernel classes offer an important level of abstraction by matching replies with their associated requests.

Once these changes have been reviewed I can drop the WIP tag and polish this up for being merged.

nikitakit · 2018-01-20T03:43:44Z

Any thoughts on the latest round of changes? (Just want to clarify that this PR is out of my hands and waiting for review)

BenRussert

Thank you for this work, looks really good overall!

The only reason I have request changes on is to clarify the test block in main.js, this does not need to be merged by my understanding.

The other thing I think we need is some better documentation so users know where to start with creating custom middleware. I wonder your thoughts on this as I could see that coming in a separate PR if you wanted to wait on making the API changes public.

Note that any time you make an api docs change you will have to npm run build:docs to generate the gitbook markdown. The changes actually need to be committed in order to be visible in the gitbook.

BenRussert · 2018-01-21T16:49:54Z

lib/main.js

+        kernel._has_been_seen_before = true;
+      }
+    });
+    // XXX(nikita): end testing


This hunk does not need to be merged since it is just for dev testing right?

BenRussert · 2018-01-21T17:09:10Z

lib/plugin-api/hydrogen-kernel.js

+
+  /*
+   * Add a kernel middleware, which allows intercepting and issuing commands to
+   * the kernel.


You could add an @param docstring here to document that this takes a Middleware object.

We will also need to document somewhere the Middleware object api users are supposed to provide. Maybe this deserves it's own gitbook page? I wonder if you have thoughts on this?

I added the param docstring.

In terms of actually documenting the API in detail, I'd like to leave that to a future PR. I'm working on some plugins for my own use, and examples would help a lot in terms of explaining what things do.

BenRussert · 2018-01-21T17:10:46Z

lib/plugin-api/hydrogen-kernel.js

+    // remove/replace some of its methods. The current plugin API is designed to
+    // ignore any such changes, which requires making a copy here. We use local
+    // variables instead of copying the entire object to make flow happy.
+    if (!!middleware.interrupt) {


Out of curiosity, is there an advantage to the double negative here?

Actually, I have no idea about this. I'm not very familiar with the edge cases of javascript typecasting.

n-riesco · 2018-01-22T10:17:47Z

@nikitakit I've gone quickly through the PR and I have a major concern about the design and minor points about naming and logging.

Please, correct me if I misunderstood. This PR introduces the concept of middleware and changes the classes HydrogenKernel, Kernel, ZMQKernel and WSKernel to implement this concept.

For the sake of separation of concerns, this middleware (or plugin) should be class of its own. And no changes should be required in Kernel, ZMQKernel and WSKernel.

Here's an approach to keep this separation of concerns:

class HydrogenKernel {
    constructor(Kernel: kernel) {this.kernel = kernel; this.plugins = []; ...;}

    prependPlugin(HydrogenPlugin: plugin) { this._plugins.unshift(plugin); }
    appendPlugin(HydrogenPlugin: plugin) { this._plugins.push(plugin); }
    removePlugin(HydrogenPlugin: plugin) { ... }

    execute(...args) {
        let newArgs = args;
        this.plugins.forEach((HydrogenPlugin: plugin) => { newArgs = plugin.execute(...newArgs); });
        this.kernel.execute(...newArgs);
    }
    inspect(...) {...}
    complete(...) {...}
    restart(...) {...}
    interrupt(...) {...}
    shutdown(...) {...}
}

And about the minor points I mentioned above:

don't use console.log or console.warn, use log
the convention in Hydrogen is to name private methods with a starting _MyMethod not rawMyMethod
since callWatches is a boolean flag, how about renaming to shouldUpdateWatches?
the name _translateExecuteResults gives the impression that this is a function that parses an execution result, instead it returns a function that parses an execution result.

nikitakit · 2018-01-22T23:53:58Z

@n-riesco Thanks for your comments! Your feedback last version was super helpful, and I appreciate that you gave v2 a look.

To add to your description of the concept of middleware, I'd like to mention that the purpose of this concept is to allow plugins to intercept and modify kernel requests/responses. Middleware needs to intercept hydrogen commands, so e.g. if you press shift-enter an execute middleware will need to be called. This makes it difficult to avoid modifying at least one of Kernel/ZMQKernel/WSKernel to support this behavior.

I say difficult because there is technically the option of creating a PluginAwareKernel class that inherits from Kernel. This approach was rejected because (1) users with plugins installed will have 100% of their kernels converted to PluginAwareKernels, which puts them on a completely different codepath from vanilla hydrogen users and increases the potential for bugs, and (2) swapping out a ZMQKernel for a PluginAwareKernel when the kernel is already associated with multiple files is needlessly complex. The simple solution is to modify the base Kernel class to accept the optional installation of middleware.

A given plugin may only wish to target particular kernels (e.g. if it implements language-specific features), so middleware is installed on a per-kernel basis instead of globally.

Once the base Kernel class supports middleware, this functionality needs to be exposed to plugins that only have access to HydrogenKernel wrapper instances. This adds a layer of indirection which is a bit annoying but does provide an opportunity to provide some sanity checking.

Regarding your chaining proposal --

    execute(...args) {
        let newArgs = args;
        this.plugins.forEach((HydrogenPlugin: plugin) => { newArgs = plugin.execute(...newArgs); });
        this.kernel.execute(...newArgs);
    }

this implementation is not sufficiently general to implement plugins that want to split one request into multiple. Consider moving hydrogen's watch functionality into a plugin: this PR enables the following approach:

myWatches = ...
kernel.addMiddleware({
  execute: (next, code, callWatches, onResults) => {
    next.execute(code, callWatches, (message, channel) => {
      onResults(message, channel);
      if (myWatches.isIdleStatusMessage(message))  {
        for (let watch of myWatches.getWatches()) {
          next.execute(watch, false, myWatches.onWatchResults);
        }
      }
    })
  }
});

The fact that next.execute can be called multiple times inside the middleware (and that you also have the option to call other methods such as next.complete) makes handling prepend/append/remove trickier, so implementing functionality other than addMiddleware is postponed.

Regarding your minor points, I will address them when I update the patchset.

n-riesco · 2018-01-23T00:20:35Z

To add to your description of the concept of middleware, I'd like to mention that the purpose of this concept is to allow plugins to intercept and modify kernel requests/responses.

Do you mean like this?

    execute(...args) {
        let newArgs = this.kernel.execute(...args);
        this.plugins.forEach((HydrogenPlugin: plugin) => { newArgs = plugin.execute(...newArgs); });
    }

n-riesco · 2018-01-23T00:22:32Z

I say difficult because there is technically the option of creating a PluginAwareKernel class that inherits from Kernel.

I don't understand what you mean by PluginAwareKernel. Could you give an example?

n-riesco · 2018-01-23T00:34:34Z

A given plugin may only wish to target particular kernels (e.g. if it implements language-specific features), so middleware is installed on a per-kernel basis instead of globally.

Something like this:

class PluginForPythonKernel {
    // ...
    attach(HydrogenKernel: hydrogenKernel) {
        if (hydrogenKernel.kernel.language === "python") {
            hydrogenKernel.appendPlugin(this);
    }
    // ...
}

n-riesco · 2018-01-23T00:42:46Z

Regarding your chaining proposal --
[...]
this implementation is not sufficiently general to implement plugins that want to split one request into multiple.
[...]
The fact that next.execute can be called multiple times inside the middleware (and that you also have the option to call other methods such as next.complete) makes handling prepend/append/remove trickier

why?

both approaches allow multiple calls to plugin.execute, plugin.complete, ...
both approaches implement a chain of plugins
both approaches are essentially equivalent; the main difference is that one of them avoids changes in Kernel, ZMQKernel and WSKernel.

nikitakit · 2018-01-23T01:22:46Z

both approaches are essentially equivalent; the main difference is that one of them avoids changes in Kernel, ZMQKernel and WSKernel.

How do you avoid modifying any of those classes?

Say I execute hydrogen:run by pressing cmd-enter. Here is what the call stack currently looks like:

Hydrogen.run [main.js]
- Hydrogen.createResultBubble [main.js]
-- Hydrogen._createResultBubble [main.js]
--- ZMQKernel.execute [zmq-kernel.js]

To insert some middleware here, I must modify main.js, zmq-kernel.js or kernel.js (by having ZMQKernel not override the execute method). This means I have to make changes in Kernel/ZMQKernel.

n-riesco · 2018-01-23T07:36:01Z

I assume the call stack would look like this:

Hydrogen.run [main.js]
- Hydrogen.createResultBubble [main.js]
-- Hydrogen._createResultBubble [main.js]
--- HydrogenKernel.execute [hydrogen-kernel.js]

nikitakit · 2018-01-24T03:26:52Z

I think that approach is inferior. As I said previously,

This approach was rejected because (1) users with plugins installed will have 100% of their kernels converted to [HydrogenKernels], which puts them on a completely different codepath from vanilla hydrogen users and increases the potential for bugs, and (2) swapping out a ZMQKernel for a [HydrogenKernel] when the kernel is already associated with multiple files is needlessly complex. The simple solution is to modify the base Kernel class to accept the optional installation of middleware.

n-riesco · 2018-01-24T09:52:48Z

(1) users with plugins installed will have 100% of their kernels converted to [HydrogenKernels], which puts them on a completely different codepath from vanilla hydrogen users and increases the potential for bugs

Since some functionality like watches will be implemented as a plugin, all users will have to use HydrogenKernel's.

(2) swapping out a ZMQKernel for a [HydrogenKernel] when the kernel is already associated with multiple files is needlessly complex.

Ditto. All users will have to use a HydrogenKernel.

The issue with this PR isn't the idea of midleware/plugin. This is an implementation issue. The classes Kernel, ZMQKernel and WSKernel know nothing about plugins, because their only purpose is to communicate with a kernel.

This PR injects the middleware/plugin functionality into Kernel, ZMQKernel and WSKernel. This means that, with this PR, Kernel, ZMQKernel and WSKernel would become responsible for communicating with kernels and plugins. From a maintenance and a design point of view this is bad because:

changes in the plugin interface would require changes in Kernel, ZMQKernel and WSKernel
Kernel, ZMQKernel and WSKernel would become more complicated, because they would have to handle both functionalities.

In the end, it all boils down to separation of concerns: "do one thing and do it will". Until now, Kernel, ZMQKernel and WSKernel do only one thing: i.e. communication with kernels.

nikitakit · 2018-01-25T20:11:30Z

Here is my plan for when we need to release a backwards-incompatible plugin API:

Create a class HydrogenKernelV2
Plugins that consume hydrogen provider 0.x get HydrogenKernel instances, plugins that consume hydrogen provider 1.x get HydrogenKernelV2 instances.
This provides a period of compatibility for older plugins.

Note that this requires that multiple HydrogenKernel wrappers to be allowed for the same kernel. There were no concerns regarding this plan when I first proposed the plugin interface.

Also, ZMQKernel and WSKernel aren't becoming more complicated: all I'm doing is renaming some of their methods.

n-riesco · 2018-01-26T00:27:16Z

Also, ZMQKernel and WSKernel aren't becoming more complicated: all I'm doing is renaming some of their methods.

I see what you've done there! Failed to mention the changes in Kernel, have you? :)

Anyway, that's not the main problem I have raised. The issue I've raised is one of design. I'm not going to repeat the problems that breaking that design will bring.

The question is: Why does this PR need to break the design of Kernel, ZMQKernel and WSKernel?

nikitakit · 2018-01-26T00:29:41Z

See my point above about plugin backward-compatibility. I consider compatibility to be of utmost importance in API design.

Edit: I actually don't care that much about changing vs. not changing Kernel (thought I think adding a new class is overkill). But I do care about being able to have multiple HydrogenKernel classes in parallel.

n-riesco · 2018-01-26T00:31:58Z

See my point above about plugin backward-compatibility. I consider compatibility to be of utmost importance in API design.

Are you arguing that it's impossible to provide plugin backward-compatibility without touching the code in Kernel, ZMQKernel and WSKernel?

nikitakit · 2018-01-26T00:33:42Z

If we have both a HydrogenKernel and HydrogenKernelV2 class in parallel, it is clear that both must be wrappers around the same hydrogen-internal object.

n-riesco · 2018-01-26T00:36:15Z

If we have both a HydrogenKernel and HydrogenKernelV2 class in parallel, it is clear that both must be wrappers around the same hydrogen-internal object.

If the logic to handle plugins is kept inside HydrogenKernel and HydrogenKernelV2, then there is no need to touch Kernel, ZMQKernel and WSKernel.

nikitakit · 2018-01-26T00:38:38Z

Note your call stack above:

Hydrogen.run [main.js]
- Hydrogen.createResultBubble [main.js]
-- Hydrogen._createResultBubble [main.js]
--- HydrogenKernel.execute [hydrogen-kernel.js]

How does HydrogenKernelV2 fit in?

In my plan, HydrogenKernel is never part of the call stack for requests not initiated by a plugin, so you can have as many parallel versions as you want.

n-riesco · 2018-01-26T01:07:48Z

How does HydrogenKernelV2 fit in?

In my plan, HydrogenKernel is never part of the call stack for requests not initiated by a plugin, so you can have as many parallel versions as you want.

This is only a naming issue. I understand you'd like to keep the class name HydrogenKernel for use by plugins consuming API v1. So let's think of another name for the class used internally: e.g. HKernel?

With this new naming, the design would be:

Kernel, ZMQKernel and WSKernel to handle communications with Jupyter kernels
HKernel combines the functionality of plugins and Kernel (and is only accessible internally)
HydrogenKernel is accessible to plugins consuming API v1 (and uses HKernel internally)
HydrogenKernelV2=HKernel is accessible to plugins consuming API v2 (and uses HKernel internally)

It's rather late here. Let's continue the discussion tomorrow.

nikitakit · 2018-01-30T00:09:59Z

@n-riesco Sorry for the delayed reply, since I was busy this weekend.

I think we're on the same page now regarding the architecture discussion; in particular that the plugin wrappers won't be used internally in a way that hurts backwards-compatibility.

I went ahead and implemented something similar to what you propose above (with the exception of naming: I use KernelTransport/Kernel instead of Kernel/HKernel to keep the diff small). This is in the last commit I pushed to this branch.

Ignoring the CI failures for a moment, let's take a look at what effect this has on the architecture. The first thing to notice is that Kernel has quite a large API surface, and when splitting it up it's not just the plugin functionality that is affected. There's nothing special about the plugin middleware: if it's going to be moved out of the Kernel class then so should other stuff like emitter or outputStore. This leaves the Kernel class to only do jupyter-protocol stuff. Alternatively, these other functions can be exposed through a getter, or referenced using kernel.kernel.XXX everywhere.

These changes produce a massive diff throughout all of hydrogen, and really has nothing to do with plugins. And I'm not even done! (see all the XXX comments) And all of this needs to be tested!

If you'd like we can proceed down this path, though it looks like it will require quite a bit of code review to happen. But to be quite honest, I don't see the value of doing such a large-scale refactoring. Especially not in a PR that's ostensibly about plugins.

nikitakit · 2018-02-14T22:58:49Z

lib/zmq-kernel.js

@@ -297,7 +297,7 @@ export default class ZMQKernel extends KernelTransport {
  onStdinMessage(message: Message) {
    log("stdin message:", message);

-    if (!this.isValidMessage(message)) {
+    if (!this._isValidMessage(message)) {


ZMQKernel still needs some validity checks because it proceeds to do things like const { msg_type } = message.header;, which assumes that message.header exists.

nikitakit · 2018-02-18T05:06:54Z

@n-riesco Please let me know how to proceed here.

Regarding your earlier points about naming (plugin vs. middleware vs. something else): I think it makes sense to keep the concepts of "Atom package that extends hydrogen" and "object that provides additional kernel functionality" distinct.

The word "plugin" is already used in the "Atom package" sense, e.g. in the hydrogen readme:

Hydrogen has support for plugins. Feel free to add your own to the list:

Hydrogen Launcher

After searching for a different word for a "kernel functionality extender", I settled on the name middleware. I'm not particularly attached to any of these names, but I do want to make sure that distinct concepts have distinct names.

Note that none of the methods execute/complete/... receive kernel as an argument, so a plugin is essentially forced to register multiple middleware objects. See here for an example.

n-riesco · 2018-02-20T16:22:34Z

@nikitakit sorry about the late reply. I'm planning to look at the latest changes tonight.

rgbkrk · 2018-02-20T17:38:22Z

While I haven't been part of the long ongoing review of this PR, I would like to say that I'm a fan of the name middleware, especially with the use of next(), since this is a common pattern in the node ecosystem, particularly express. The only big requirement I'd see is that we define the terms within our own internal developer documentation for Hydrogen. We can always change names later if they're too confusing. That shouldn't be what holds this PR up.

n-riesco · 2018-02-21T00:23:26Z

@nikitakit

[...], but I do want to make sure that distinct concepts have distinct names.

If we really think that the case of plugins that don't register a middleware will be common, then fair enough.

But to my mind, plugin and middleware are essentially the same concept.

After searching for a different word for a "kernel functionality extender", I settled on the name middleware. I'm not particularly attached to any of these names, [...]

I wouldn't say that the middleware defined in this API extends the kernel functionality, but instead, I'd said this middleware transforms kernel requests and replies. So how about we call it transform?

Please let me know how to proceed here.

I had a another read of the the latest changes and here's a list of the remaining blocking points:

Have you tested the functionality for input_request? I'm unclear how the new implementation could work.
For me, that the new API exposes callWatches to the plugins is still a blocking point, because this prevents the implementation of Watches as a plugin.

nikitakit · 2018-02-21T02:11:30Z

@n-riesco

I just pushed an update that gets rid of the callWatches argument from the plugin API.

Note that watches have apparently always triggered on "status: idle" messages and not on "execute_reply" messages. I'm not in a position to know if changing this would break things, so I've retained the old behavior.

I tested the input_request behavior (both previously and after this latest round of changes), and there don't appear to be any impacts from a user-facing perspective.

There isn't currently an API for plugins to actively participate in the input_request/input_reply exchange because there hasn't been a convincing usecase for it. If a plugin wants to query the user it can just use Atom's DOM API.

Before designing this plugin API, I went through the entire hydrogen issue tracker to try to cover a wide range of real use-cases that have been suggested. I didn't find anything that would require interacting with inputs, and I'm not a fan of designing APIs for "what-if" scenarios without any real applications in mind.

n-riesco · 2018-02-21T07:45:23Z

@nikitakit

Note that watches have apparently always triggered on "status: idle" messages and not on "execute_reply" messages. I'm not in a position to know if changing this would break things, so I've retained the old behavior.

The Jupyter messaging protocol requires that an execute_request is closed with an execute_reply.

Kernels, like IJavascript, that can run requests asynchronously are able to accept new requests (i.e. they can send status:idle) before and execution_request has been closed (i.e. before an execute_reply has been sent).

Could you point me to code in master where the behaviour "Note that watches have apparently always triggered on "status: idle" messages and not on "execute_reply" messages." is implemented?

I tested the input_request behavior (both previously and after this latest round of changes), and there don't appear to be any impacts from a user-facing perspective.

Could you explain how the new implementation works? I'm confused because the only code I've found that opens an input dialog has a comment that says it never runs:

https://github.com/nikitakit/hydrogen/blob/a00cb945eef6b37f8a73b2f0e4422ddaccceb70c/lib/zmq-kernel.js#L313-L333

There isn't currently an API for plugins to actively participate in the input_request/input_reply exchange because there hasn't been a convincing usecase for it. If a plugin wants to query the user it can just use Atom's DOM API.

I don't understand the meaning of this paragraph. The new API seems to call the plugins when an input_request is received.

nikitakit · 2018-02-21T07:51:38Z

Could you point me to code in master where the behaviour "Note that watches have apparently always triggered on "status: idle" messages and not on "execute_reply" messages." is implemented?

I don't understand the meaning of this paragraph. The new API seems to call the plugins when an input_request is received.

You're right, I wasn't quite precise. Plugins have the option of modifying the input_request, but well generally just pass it along. However, they do not currently have the option to do anything with input_reply messages.

nikitakit · 2018-02-21T07:53:08Z

lib/kernel.js

-    return result;
+        // TODO(nikita): perhaps it would make sense to install middleware for
+        // sending input replies
+        const inputView = new InputView({ prompt }, (input: string) =>


Input requests are handled here. (Note that GitHub by default doesn't render the diff for this file because it's too big)

n-riesco · 2018-02-21T08:47:17Z

@nikitakit

Could you point me to code in master where the behaviour "Note that watches have apparently always triggered on "status: idle" messages and not on "execute_reply" messages." is implemented?
ZMQKernel: https://github.com/nteract/hydrogen/blob/master/lib/zmq-kernel.js#L363
WSKernel: https://github.com/nteract/hydrogen/blob/master/lib/ws-kernel.js#L53

Oh, so what you mean is that the Watch callbacks have always been triggered on 'status:idle`. I think that's the right thing to do. This PR keeps doing the same, doesn't it?

I don't understand the meaning of this paragraph. The new API seems to call the plugins when an input_request is received.

You're right, I wasn't quite precise. Plugins have the option of modifying the input_request, but well generally just pass it along. However, they do not currently have the option to do anything with input_reply messages.

Couldn't a plugin hijack an input_request? I.e. when the plugin gets an input_request, instead of invoking next, it can invoke the onResults callback with a fake input_reply.

Input requests are handled here. (Note that GitHub by default doesn't render the diff for this file because it's too big)

bad github! :(

OK, now I understand how it works. I'd remove the lines https://github.com/nikitakit/hydrogen/blob/a00cb945eef6b37f8a73b2f0e4422ddaccceb70c/lib/zmq-kernel.js#L314-L331 . Everywhere else in ZMQKernel we drop invalid messages. We certainly could add a log message, but ZMQKernel shouldn't do any UI (only hadnle comms with kernel).

nikitakit · 2018-02-21T08:51:27Z

This PR keeps doing the same, doesn't it?

Yes, the functionality is unchanged.

I.e. when the plugin gets an input_request, instead of invoking next, it can invoke the onResults callback with a fake input_reply.

input_request is from the kernel, to the frontend. So input_request is what gets passed to onResults.

nikitakit · 2018-02-21T08:56:21Z

lib/zmq-kernel.js

+    if (callback) {
+      callback(message, "stdin");
+    } else {
+      // This branch of code should technically never run, but we keep it just


@n-riesco So you think we should drop this code now, instead of waiting for a few releases?

I guess I thought a deprecation-notice approach will catch if there are any non-compliant kernels out there that would break as a result of removing this functionality. (In the master branch, all input requests are replied to, even if they are not in response to an execute request.)

Is this a hypothetical question? Are you aware of any kernels setting the wrong msg_id in input_request?

To my understanding this is a bug in the kernel, and I wouldn't like Hydrogen to corrupt the spec of input_request any further (input_reply messages are already inconsistent with the rest of the Jupyter protocol: in my opinion, input_reply should set parent_header as it's done for all the other reply messages).

I'm not aware of any kernels doing this. I'll push a commit to get rid of these lines.

Side note: there's a race condition if the very first line of Python code you run in hydrogen is input(). It looks unrelated to this PR, but worth looking into at some point.

Edit: can't reproduce anymore, but maybe it's still there

@nikitakit

there's a race condition if the very first line of Python code you run in hydrogen is input().

I don't see the race. Isn't this.executionCallbaks[msg_id] created in the same method that sends the input() execution request (and thus, before any replies)? Here.

I have no idea because I can't trigger it anymore. But I swear it happened a few times in a row.

My best guess is that when a new kernel is created and a blue "Hydrogen Kernels updated:" box appears, the input box never gets rendered. But I can't reproduce anymore.

So, how about leaving the log message so that we can debug the race the next time it happens?

It's completely unrelated to this code change.

Here's a simple issue:

Run input() in Python

Wait for input prompt to appear

Activate the command panel using cmd-shift-p

Watch the input prompt go away permanently

I think the race is probably with some graphics-related event (similar to the command panel activation via cmd-shift-p)

Would you open an issue for it (so we don't forget)?

n-riesco · 2018-02-21T09:12:48Z

@nikitakit

I.e. when the plugin gets an input_request, instead of invoking next, it can invoke the onResults callback with a fake input_reply.

input_request is from the kernel, to the frontend. So input_request is what gets passed to onResults.

yes, you're right.

Let me re-word the idea: a plugin could hijack an input_request (received via its OnResults callback) by invoking next.inputReply with a fake input_reply, instead of calling the parent's onResult callback.

nikitakit · 2018-02-21T09:14:02Z

Yes, we could create a next.inputReply. I have postponed that for a future PR because I haven't found any usecases for it.

n-riesco · 2018-02-21T09:18:56Z

@nikitakit

Yes, we could create a next.inputReply.

Again, you're right (I thought inputReply was part of the middleware interface).

I have postponed that for a future PR because I haven't found any usecases for it.

yes, I understand.

n-riesco · 2018-02-21T09:37:50Z

@nikitakit a00cb94 addresses the last of the blocking issues. I think this PR is ready to merge (but note I've only read the code; I haven't tested it in my computer).

nikitakit · 2018-02-21T09:52:45Z

@n-riesco Thank you for your time in looking over this code!

I've tested the code and not found any breakage to existing functionality. I also have a sample plugin that appears to run without issue.

(Someone else will need to merge this PR since I'm not part of the nteract org)

n-riesco · 2018-02-21T10:01:52Z

@nikitakit

(Someone else will need to merge this PR since I'm not part of the nteract org)

Oh, I thought you were!

Since this is a significant PR, before merging, I'd like to confirm that @lgeiger @BenRussert and @rgbkrk are happy with the changes.

lgeiger

Great to see this coming along!

@nikitakit @n-riesco Thanks for the great work and review here 🎉

Let's get this merged! We can always iron out the details in future PRs.

lgeiger · 2018-02-21T17:29:49Z

Woo it's on 🎉

rgbkrk · 2018-02-21T17:54:53Z

(Someone else will need to merge this PR since I'm not part of the nteract org)

You're always welcome back with 2FA. In the meantime, I'll continue mentally thinking of you as in the org. 😄

Delete useless _happy boolean

82d2dff

nikitakit commented Jan 5, 2018

View reviewed changes

lgeiger reviewed Jan 7, 2018

View reviewed changes

nikitakit force-pushed the nikita/plugin-updates-2 branch from fbd88ce to acd88f8 Compare January 12, 2018 02:52

BenRussert requested changes Jan 21, 2018

View reviewed changes

nikitakit commented Feb 14, 2018

View reviewed changes

Add message validity checks to MiddlewareAdapter

d43b0e4

nikitakit force-pushed the nikita/plugin-updates-2 branch from 41e7c29 to d43b0e4 Compare February 15, 2018 18:17

Remove callWatches argument from plugin API

a00cb94

nikitakit changed the title ~~[WIP] Update Plugin API [v2]~~ Update Plugin API [v2] Feb 21, 2018

nikitakit commented Feb 21, 2018

View reviewed changes

Ignore input_request messages without a valid parent

7ec531d

lgeiger approved these changes Feb 21, 2018

View reviewed changes

BenRussert approved these changes Feb 21, 2018

View reviewed changes

n-riesco merged commit c9826b1 into nteract:master Feb 21, 2018

n-riesco mentioned this pull request Jun 21, 2018

How to call Hydrogen from outside of Atom? #1342

Closed

nikitakit mentioned this pull request Mar 14, 2019

Detect watch being executed from a middleware #1568

Open

Update Plugin API [v2] #1179

Update Plugin API [v2] #1179

Conversation

nikitakit commented Jan 5, 2018 • 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

lgeiger 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

nikitakit commented Jan 8, 2018

BenRussert commented Jan 11, 2018

nikitakit commented Jan 12, 2018

nikitakit commented Jan 20, 2018

BenRussert 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

n-riesco commented Jan 22, 2018 • edited Loading

nikitakit commented Jan 22, 2018

n-riesco commented Jan 23, 2018

n-riesco commented Jan 23, 2018

n-riesco commented Jan 23, 2018

n-riesco commented Jan 23, 2018

nikitakit commented Jan 23, 2018

n-riesco commented Jan 23, 2018

nikitakit commented Jan 24, 2018

n-riesco commented Jan 24, 2018 • edited Loading

nikitakit commented Jan 25, 2018

n-riesco commented Jan 26, 2018

nikitakit commented Jan 26, 2018 • edited Loading

n-riesco commented Jan 26, 2018

nikitakit commented Jan 26, 2018

n-riesco commented Jan 26, 2018 • edited Loading

nikitakit commented Jan 26, 2018

n-riesco commented Jan 26, 2018

nikitakit commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

nikitakit commented Feb 18, 2018

n-riesco commented Feb 20, 2018

rgbkrk commented Feb 20, 2018

n-riesco commented Feb 21, 2018 • edited Loading

nikitakit commented Feb 21, 2018

n-riesco commented Feb 21, 2018

nikitakit commented Feb 21, 2018

Choose a reason for hiding this comment

n-riesco commented Feb 21, 2018

nikitakit commented Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitakit Feb 21, 2018 • 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

n-riesco commented Feb 21, 2018

nikitakit commented Feb 21, 2018

n-riesco commented Feb 21, 2018

n-riesco commented Feb 21, 2018

nikitakit commented Feb 21, 2018

n-riesco commented Feb 21, 2018

lgeiger left a comment

Choose a reason for hiding this comment

lgeiger commented Feb 21, 2018

rgbkrk commented Feb 21, 2018 • edited Loading

nikitakit commented Jan 5, 2018 •

edited

Loading

n-riesco commented Jan 22, 2018 •

edited

Loading

n-riesco commented Jan 24, 2018 •

edited

Loading

nikitakit commented Jan 26, 2018 •

edited

Loading

n-riesco commented Jan 26, 2018 •

edited

Loading

nikitakit commented Jan 30, 2018 •

edited

Loading

n-riesco commented Feb 21, 2018 •

edited

Loading

nikitakit commented Feb 21, 2018 •

edited

Loading

nikitakit Feb 21, 2018 •

edited

Loading

rgbkrk commented Feb 21, 2018 •

edited

Loading